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.

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

fix(env): parsing --env incorrect in cli #19560

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

BlackHole1
Copy link
Contributor

@BlackHole1 BlackHole1 commented Aug 9, 2023

Fixed: #19096 (comment)
Fixed: #19096 (comment)

The root cause of this issue lies in the fact that the shell discards the quotes in the parameters, as shown in the following code snippet:

podman run -e K='a#b' alpine printenv aaa
# To
podman run -e K=a#b alpine printenv aaa

This will result in problems with the current parseEnv implementation. Unfortunately, there is no way to change the behavior of the shell, so the only option is to fix it within parseEnv. In order to reduce code complexity, I have ported the previous parseEnv function back and renamed it as parseEnvWithSlice. Furthermore, this function will be used exclusively for the --env option, which will bring another benefit: it will run faster than --env-file.

Now:

$ podman run --env aaa='aaa\nbbb#ccc\nddd' alpine printenv aaa
aaa\nbbb#ccc\nddd
44-09 13 44@2x

Does this PR introduce a user-facing change?

Fix the issue where the parsing of the `--env` parameter value is incorrect.

Ref:

  1. pass json string as flag remove double quotes spf13/cobra#898
  2. https://stackoverflow.com/questions/53896081/how-to-keep-quotes-in-os-args
  3. os.Args is missing/eating/interpreting double quote on Windows golang/go#30952

PTAL @rhatdan @vrothberg @Luap99
/cc @edsantiago @kajinamit

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 9, 2023

@BlackHole1: GitHub didn't allow me to request PR reviews from the following users: kajinamit.

Note that only containers members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Fixed: #19096 (comment)
Fixed: #19096 (comment)

The root cause of this issue lies in the fact that the shell discards the quotes in the parameters, as shown in the following code snippet:

podman run -e K='a#b' alpine printenv aaa
# To
podman run -e K=a#b alpine printenv aaa

This will result in problems with the current parseEnv implementation. Unfortunately, there is no way to change the behavior of the shell, so the only option is to fix it within parseEnv. In order to reduce code complexity, I have ported the previous parseEnv function back and renamed it as parseEnvWithSlice. Furthermore, this function will be used exclusively for the --env option, which will bring another benefit: it will run faster than --env-file.

Does this PR introduce a user-facing change?

Fix the issue where the parsing of the `--env` parameter value is incorrect.

Ref:

  1. pass json string as flag remove double quotes spf13/cobra#898
  2. https://stackoverflow.com/questions/53896081/how-to-keep-quotes-in-os-args
  3. os.Args is missing/eating/interpreting double quote on Windows golang/go#30952

PTAL @rhatdan @vrothberg @Luap99
/cc @edsantiago @kajinamit

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mheon mheon added the 4.6 label Aug 9, 2023
pkg/env/env_test.go Show resolved Hide resolved
pkg/env/env_test.go Outdated Show resolved Hide resolved
@BlackHole1
Copy link
Contributor Author

@vrothberg Thank you for your CR. I modified done.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 9, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BlackHole1, vrothberg

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

The pull request process is described here

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 2023
@rhatdan
Copy link
Member

rhatdan commented Aug 9, 2023

/lgtm
/hold
@BlackHole1 PTAL

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Aug 9, 2023
@BlackHole1
Copy link
Contributor Author

/lgtm /hold @BlackHole1 PTAL

@rhatdan Are you pinging the wrong person? 🤣

@rhatdan
Copy link
Member

rhatdan commented Aug 9, 2023

Yes. I meant to ping the reporter of the bug.
/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 9, 2023
@openshift-merge-robot openshift-merge-robot merged commit 57fac93 into containers:main Aug 9, 2023
85 checks passed
@BlackHole1 BlackHole1 deleted the fix branch August 9, 2023 12:50
@TomSweeneyRedHat
Copy link
Member

I'm going to backport this to the 4.6 branch.

@edsantiago
Copy link
Member

I don't think the original env-file PR made it into 4.6

@edsantiago
Copy link
Member

I'm upset that this got merged without my review. There are still problems with env-file processing. I will open up new issues.

@TomSweeneyRedHat
Copy link
Member

Just noting for future reference, that if this fix is backported into Podman 4.4, it has the potential to break OpenStack on that version of RHEL (9.2). Please do NOT backport this to the 4.4 branch without very careful consideration. Please note: https://bugzilla.redhat.com/show_bug.cgi?id=2230212 for details.

@kajinamit
Copy link

What breaks OpenStack(actually it does not break the whole OpenStack but only TripleO which is the deployment tool) is not this one but #19096 , thought as long as that is backported along with this follow-up PR then it would not cause problem with the usage in RHEL9.2 .

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Nov 10, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants