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

Minor fix splitting env vars in podman-commit #3138

Merged
merged 1 commit into from
May 19, 2019

Conversation

weirdwiz
Copy link
Collaborator

string.Split() splits into slice of size greater than 2
which may result in loss of environment variables

fixes #3132

@rh-atomic-bot
Copy link
Collaborator

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

@mheon
Copy link
Member

mheon commented May 16, 2019

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon, weirdwiz

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 16, 2019
@mheon
Copy link
Member

mheon commented May 16, 2019

/ok-to-test

@mheon
Copy link
Member

mheon commented May 16, 2019

Code LGTM

@rhatdan
Copy link
Member

rhatdan commented May 16, 2019

@weirdwiz Could you add a quick test to verify that the reported issue is fixed by this and will not happen again.

@rhatdan
Copy link
Member

rhatdan commented May 16, 2019

Wrong issue.

@TomSweeneyRedHat
Copy link
Member

Dang @weirdwiz , your fingers are faster than mine. I'd not realized you'd been assigned this one and was just about to spin up a PR. I've tested and this fixes the issue. A new test wouldn't hurt any.
LGTM other than the test.

@weirdwiz
Copy link
Collaborator Author

@rhatdan I have added the test

I cannot figure out why you are getting permission denied on your system, I have only changed a line regarding splitting the string in the commit command, I don't see how this could affect the run command.

@TomSweeneyRedHat
Copy link
Member

@weirdwiz ignore @rhatdan's perm comment, he'd too many windows up at once and put a comment here in this PR that was meant for another project entirely. Thanks for the added test.

@TomSweeneyRedHat
Copy link
Member

LGTM assuming happy tests.

`string.Split()` splits into slice of size greater than 2
which may result in loss of environment variables

fixes containers#3132

Signed-off-by: Divyansh Kamboj <kambojdivyansh2000@gmail.com>
@weirdwiz
Copy link
Collaborator Author

All tests passing now :D

@mheon
Copy link
Member

mheon commented May 19, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 19, 2019
@openshift-merge-robot openshift-merge-robot merged commit ce84c3a into containers:master May 19, 2019
@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 Sep 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 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. ok-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Podman loses env-var values on commit
7 participants