Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: env vars with "=" in their values ignored for sidecar CMPs #8792 #8793

Merged
merged 1 commit into from
Mar 23, 2022
Merged

fix: env vars with "=" in their values ignored for sidecar CMPs #8792 #8793

merged 1 commit into from
Mar 23, 2022

Conversation

ilya-lesikov
Copy link
Contributor

@ilya-lesikov ilya-lesikov commented Mar 16, 2022 •
edited
Loading

Fixes #8792

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

@ilya-lesikov
Copy link
Contributor Author

I see this commit from yesterday incidentally factored out this function, could this be already fixed? Not sure, but I'll wait for 41db812 to land in a new release and will test it out.

Marking this as a Draft for now.

@ilya-lesikov ilya-lesikov marked this pull request as draft March 16, 2022 15:53
@crenshaw-dev
Copy link
Collaborator

@ilya-lesikov that PR didn't fix it, just moved the function from one place to another. Want to go ahead and mark this as ready for review? I'll request Leo's review since he did that refactor and might have some additional context.

@ilya-lesikov ilya-lesikov marked this pull request as ready for review March 21, 2022 10:57
@ilya-lesikov
Copy link
Contributor Author

@crenshaw-dev Hi, marked as ready.

@crenshaw-dev
Copy link
Collaborator

@ilya-lesikov sorry to keep bothering for a simple change, but could you merge master?

If you prefer, I can open a new PR w/ the same changes and handle the nitpicky stuff. Either way works for me. :-)

@ilya-lesikov
fix: env var values with equal sign ignored
cc9a26b 
Environment variable with "=" in its value is not passed to sidecar
config management plugin, but ignored.

Signed-off-by: Ilya Lesikov <ilya@lesikov.com>
@ilya-lesikov ilya-lesikov force-pushed the fix-env-vals-with-equal-signs-ignored branch from 92a82c7 to cc9a26b Compare March 22, 2022 13:10
@codecov
Copy link

codecov bot commented Mar 22, 2022 •
edited
Loading

Codecov Report

Merging #8793 (cc9a26b) into master (a6c664b) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #8793   +/-   ##
=======================================
  Coverage   42.87%   42.87%           
=======================================
  Files         186      186           
  Lines       23335    23335           
=======================================
  Hits        10006    10006           
  Misses      11898    11898           
  Partials     1431     1431
Impacted Files Coverage Δ
util/cmp/stream.go 51.04% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6c664b...cc9a26b. Read the comment docs.

crenshaw-dev
crenshaw-dev approved these changes Mar 22, 2022
View reviewed changes
Copy link
Collaborator

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again! Requesting Leo's review since he recently did some refactoring near this code.

leoluz
leoluz approved these changes Mar 23, 2022
View reviewed changes
Copy link
Collaborator

@leoluz leoluz left a comment

Choose a reason for hiding this comment

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

LGTM

@leoluz leoluz merged commit 19cff7f into argoproj:master Mar 23, 2022
wojtekidd pushed a commit to wojtekidd/argo-cd that referenced this pull request Apr 25, 2022
@ilya-lesikov @wojtekidd
fix: env var values with equal sign ignored (argoproj#8793)
8c22835 
Environment variable with "=" in its value is not passed to sidecar
config management plugin, but ignored.

Signed-off-by: Ilya Lesikov <ilya@lesikov.com>
Signed-off-by: wojtekidd <wojtek.cichon@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leoluz leoluz leoluz approved these changes

@crenshaw-dev crenshaw-dev crenshaw-dev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Environment variables with "=" in their values ignored for sidecar CMPs
3 participants
@ilya-lesikov @crenshaw-dev @leoluz