Skip to content

Fix duplicate environment variables in MergeEnvVars - #2338

Open
Shubbu03 wants to merge 2 commits into
shipwright-io:mainfrom
Shubbu03:feat/fix-env-merge-duplicates
Open

Fix duplicate environment variables in MergeEnvVars#2338
Shubbu03 wants to merge 2 commits into
shipwright-io:mainfrom
Shubbu03:feat/fix-env-merge-duplicates

Conversation

@Shubbu03

@Shubbu03 Shubbu03 commented Sep 7, 2026

Copy link
Copy Markdown

Changes

Fix env.MergeEnvVars so duplicate environment variable names within the incoming slice are detected correctly.

When overwriting is disabled, duplicates now return an error. When enabled, the latest value replaces the earlier one while preserving order. Validation also runs when the destination slice is empty.

Added regression tests for duplicate values, ValueFrom, empty destinations, ordering, and forbidden variables.

Related Issue

Fixes #2330

Type of PR

/kind bug

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Kind label has been set
  • Release notes block has been filled in, or marked NONE

Release Notes

Fixed duplicate environment variable handling in env.MergeEnvVars, preventing duplicate variables from appearing in generated build execution resources.

@pull-request-size pull-request-size Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 7, 2026
@openshift-ci openshift-ci Bot added kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 7, 2026
@openshift-ci
openshift-ci Bot requested a review from apoorvajagtap September 7, 2026 09:42
@openshift-ci

openshift-ci Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign qu1queee for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kaizakin

kaizakin commented Sep 7, 2026

Copy link
Copy Markdown
Member

@Shubbu03 please sign your commits

also please check the release note against the template

Signed-off-by: Shubbu03 <thatcoderguyshubham@gmail.com>
@Shubbu03
Shubbu03 force-pushed the feat/fix-env-merge-duplicates branch from fe5df65 to d6ee5b1 Compare September 7, 2026 09:53
@openshift-ci openshift-ci Bot added release-note Label for when a PR has specified a release note and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 7, 2026
@Shubbu03

Shubbu03 commented Sep 7, 2026

Copy link
Copy Markdown
Author

Thanks @kaizakin — both items should be addressed now:

  • The commit is signed off.
  • The release-note block is also fixed.

@kaizakin

kaizakin commented Sep 7, 2026

Copy link
Copy Markdown
Member

/ok-to-test

@openshift-ci openshift-ci Bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Sep 7, 2026

@kaizakin kaizakin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have to do some dedup check for into as well
because as of now suppose from is empty, function returns in the first if check without considering whether into has duplicates

even if the control moves down, constructing a map with into alone overwrites the env variable considering it has duplicates (which we shouldn't do if the overwritevalues is set to false)

Comment thread pkg/env/env.go Outdated
Signed-off-by: Shubbu03 <thatcoderguyshubham@gmail.com>
@Shubbu03
Shubbu03 force-pushed the feat/fix-env-merge-duplicates branch from 2b79aa2 to 12f6938 Compare September 7, 2026 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Categorizes issue or PR as related to a bug. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Label for when a PR has specified a release note size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

env.MergeEnvVars allows duplicate variables from the same input

3 participants