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

Ensure DefaultEnvVariables is used in Specgen #7343

Merged
merged 1 commit into from
Aug 19, 2020

Conversation

mheon
Copy link
Member

@mheon mheon commented Aug 17, 2020

When we rewrote Podman's pkg/spec, one of the things that was lost was our use of a set of default environment variables, that ensure all containers have at least $PATH and $TERM set.

While we're in the process of re-adding it, change it from a variable to a function, so we can ensure the Join function does not overwrite it and corrupt the defaults.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 17, 2020
@mheon
Copy link
Member Author

mheon commented Aug 17, 2020

env := map[string]string{
"container": "podman",
}
env := envLib.DefaultEnvVariables()
Copy link
Member

Choose a reason for hiding this comment

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

It should be getting these fields from containers/common

Copy link
Member Author

Choose a reason for hiding this comment

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

We can change that in a future release; for now, we need this merged so we can get it in 2.0.5

Copy link
Member Author

Choose a reason for hiding this comment

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

Refactored a bit.

I don't think these make sense for containers.conf, we want to ensure a few basic environment variables are always populated - the user shouldn't really need to ever set this, outside of the existing Env section of containers.conf.

@mheon mheon force-pushed the update_defaultenv branch 2 times, most recently from 20cc2b3 to 24a74c8 Compare August 17, 2020 21:16
Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

LGTM
assuming there's a jira card or podman issue for doing the work that @rhatdan was asking for.

When we rewrote Podman's pkg/spec, one of the things that was
lost was our use of a set of default environment variables, that
ensure all containers have at least $PATH and $TERM set.

While we're in the process of re-adding it, change it from a
variable to a function, so we can ensure the Join function does
not overwrite it and corrupt the defaults.

Signed-off-by: Matthew Heon <[email protected]>
@mheon mheon force-pushed the update_defaultenv branch from 24a74c8 to a7e864e Compare August 18, 2020 19:17
@mheon
Copy link
Member Author

mheon commented Aug 18, 2020

@rhatdan Updated to remove parts that conflict with #7355 - PTAL

@rhatdan
Copy link
Member

rhatdan commented Aug 18, 2020

/lgtm
/approve
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 18, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 18, 2020
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@mheon
Copy link
Member Author

mheon commented Aug 19, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 19, 2020
@openshift-merge-robot openshift-merge-robot merged commit 45b3d61 into containers:master Aug 19, 2020
@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 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants