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

Set engine env from common config #6790

Merged
merged 1 commit into from
Jul 6, 2020

Conversation

QiWang19
Copy link
Contributor

@QiWang19 QiWang19 commented Jun 26, 2020

vendor commond PR containers/common#198 to set env

related issue containers/common#31

Signed-off-by: Qi Wang [email protected]

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 26, 2020
@QiWang19 QiWang19 changed the title [WIP]Set engine env from common config Set engine env from common config Jul 1, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 1, 2020
@QiWang19
Copy link
Contributor Author

QiWang19 commented Jul 2, 2020

@rhatdan @mheon PTAL

value := ""
if len(splitEnv) == 2 {
value = splitEnv[1]
}
Copy link
Member

Choose a reason for hiding this comment

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

If it is not 2, this should also be an error.

Copy link
Member

Choose a reason for hiding this comment

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

I see what you did, but users might accidentally override an existing environement variable with nothing.
If they want nothing then they should set it
foo=""

@QiWang19 QiWang19 force-pushed the set_engine_env branch 2 times, most recently from 38b7929 to 11b5bec Compare July 2, 2020 20:19
@rhatdan
Copy link
Member

rhatdan commented Jul 2, 2020

@@ -128,6 +128,16 @@ func persistentPreRunE(cmd *cobra.Command, args []string) error {
return err
}

for _, env := range cfg.Engine.Env {
splitEnv := strings.SplitN(env, "=", 2)
Copy link
Member

Choose a reason for hiding this comment

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

Do we want this to take precedence over actual OS environment? I imagine that if I have something set in containers.conf but later do a ENVVAR=value podman run ... I want the version passed there to take precedence.

If so, we should only define environment variables that are not already defined.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I guess that makes sense. @QiWang19 Please add a note to the man pages on podman (buildah) about this, also, just do a check to see if environment is set, it set write a debug message, if different, and don't override,

cmd/podman/root.go Outdated Show resolved Hide resolved
@QiWang19 QiWang19 force-pushed the set_engine_env branch 2 times, most recently from 456f9ef to 6797511 Compare July 2, 2020 23:21
Set the env that is used by Podman.
related issue containers/common#31

Signed-off-by: Qi Wang <[email protected]>
@QiWang19
Copy link
Contributor Author

QiWang19 commented Jul 6, 2020

@rhatdan @mheon PTAL

@rhatdan
Copy link
Member

rhatdan commented Jul 6, 2020

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 6, 2020
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: QiWang19, 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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 6, 2020
@openshift-merge-robot openshift-merge-robot merged commit 4bdc119 into containers:master Jul 6, 2020
@QiWang19 QiWang19 deleted the set_engine_env branch July 6, 2020 14:40
QiWang19 added a commit to QiWang19/buildah that referenced this pull request Jul 10, 2020
Set engine env from containrs.conf. Mirror podman PR containers/podman#6790.

Signed-off-by: Qi Wang <[email protected]>
QiWang19 added a commit to QiWang19/buildah that referenced this pull request Jul 13, 2020
Set engine env from containers.conf. Mirror podman PR containers/podman#6790.

Signed-off-by: Qi Wang <[email protected]>
QiWang19 added a commit to QiWang19/buildah that referenced this pull request Jul 14, 2020
Set engine env from containers.conf. Mirror podman PR containers/podman#6790.

Signed-off-by: Qi Wang <[email protected]>
QiWang19 added a commit to QiWang19/buildah that referenced this pull request Jul 14, 2020
Set engine env from containers.conf. Mirror podman PR containers/podman#6790.

Signed-off-by: Qi Wang <[email protected]>
bors bot added a commit to containers/buildah that referenced this pull request Jul 15, 2020
2457: Set engine env from containers.conf r=rhatdan a=QiWang19

Set engine env from containrs.conf. Mirror podman PR containers/podman#6790.

Signed-off-by: Qi Wang <[email protected]>

<!--
Thanks for sending a pull request!

Please make sure you've read and understood our contributing guidelines
(https://github.com/containers/buildah/blob/master/CONTRIBUTING.md) as well as ensuring
that all your commits are signed with `git commit -s`.
-->

#### What type of PR is this?

<!--
Please label this pull request according to what type of issue you are
addressing, especially if this is a release targeted pull request.

Uncomment only one `/kind <>` line, hit enter to put that in a new line, and
remove leading whitespace from that line:
-->

> /kind api-change
> /kind bug
> /kind cleanup
> /kind deprecation
> /kind design
> /kind documentation
> /kind failing-test 
> /kind feature
> /kind flake
> /kind other

#### What this PR does / why we need it:

#### How to verify it

#### Which issue(s) this PR fixes:

<!--
Automatically closes linked issue when PR is merged.
Uncomment the following comment block and include the issue
number or None on one line.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`, or `None`.
-->

<!--
Fixes #
or
None
-->

#### Special notes for your reviewer:

#### Does this PR introduce a user-facing change?

<!--
If no, just write `None` in the release-note block below. If yes, a release note
is required: Enter your extended release note in the block below. If the PR
requires additional action from users switching to the new release, include the
string "action required".

For more information on release notes please follow the kubernetes model:
https://git.k8s.io/community/contributors/guide/release-notes.md
-->

```release-note

```



Co-authored-by: Qi Wang <[email protected]>
bors bot added a commit to containers/buildah that referenced this pull request Jul 15, 2020
2457: Set engine env from containers.conf r=rhatdan a=QiWang19

Set engine env from containrs.conf. Mirror podman PR containers/podman#6790.

Signed-off-by: Qi Wang <[email protected]>

<!--
Thanks for sending a pull request!

Please make sure you've read and understood our contributing guidelines
(https://github.com/containers/buildah/blob/master/CONTRIBUTING.md) as well as ensuring
that all your commits are signed with `git commit -s`.
-->

#### What type of PR is this?

<!--
Please label this pull request according to what type of issue you are
addressing, especially if this is a release targeted pull request.

Uncomment only one `/kind <>` line, hit enter to put that in a new line, and
remove leading whitespace from that line:
-->

> /kind api-change
> /kind bug
> /kind cleanup
> /kind deprecation
> /kind design
> /kind documentation
> /kind failing-test 
> /kind feature
> /kind flake
> /kind other

#### What this PR does / why we need it:

#### How to verify it

#### Which issue(s) this PR fixes:

<!--
Automatically closes linked issue when PR is merged.
Uncomment the following comment block and include the issue
number or None on one line.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`, or `None`.
-->

<!--
Fixes #
or
None
-->

#### Special notes for your reviewer:

#### Does this PR introduce a user-facing change?

<!--
If no, just write `None` in the release-note block below. If yes, a release note
is required: Enter your extended release note in the block below. If the PR
requires additional action from users switching to the new release, include the
string "action required".

For more information on release notes please follow the kubernetes model:
https://git.k8s.io/community/contributors/guide/release-notes.md
-->

```release-note

```



Co-authored-by: Qi Wang <[email protected]>
@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.

6 participants