Skip to content
New issue

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

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

Already on GitHub? # to your account

Add back setting environment variable #3549

Merged
merged 2 commits into from
Jul 27, 2020

Conversation

tengqm
Copy link
Contributor

@tengqm tengqm commented Jul 26, 2020

Description of the change:

The commit ed92d3668ca921270d01055d8d8e01f3fd5187d3 removed the setAnsibleEnvVars function. However, that function is important to make sure the command line flags ansible-roles-path and ansible-collections-path work. These command line flags are set into environment variables that will be picked up later.

Motivation for the change:

This PR fixes an issue introduced by a previous PR.

Not sure we need a change log for this kind of fix, because it is fixing a bug that is not yet released.

Comment on lines +126 to +130
err = setAnsibleEnvVars(f)
if err != nil {
log.Error(err, "Failed to set environment variable.")
os.Exit(1)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
err = setAnsibleEnvVars(f)
if err != nil {
log.Error(err, "Failed to set environment variable.")
os.Exit(1)
}
if err := setAnsibleEnvVars(f); err != nil {
log.Error(err, "Failed to set environment variable.")
os.Exit(1)
}

"value", f.AnsibleCollectionsPath)
}
return nil
}
Copy link
Contributor

@camilamacedo86 camilamacedo86 Jul 26, 2020

Choose a reason for hiding this comment

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

I agree. Shows that it should be kept. @fabianvf @asmacdo @jmrodri @joelanford wdyt?

@camilamacedo86 camilamacedo86 added this to the v1.0.0 milestone Jul 26, 2020
@camilamacedo86 camilamacedo86 added the language/ansible Issue is related to an Ansible operator project label Jul 26, 2020
@joelanford
Copy link
Member

Great catch @tengqm! Thanks!

This was actually released in v0.19.0, so we'll need to backport this. Because of this, can you please add a CHANGELOG fragment?

tengqm and others added 2 commits July 27, 2020 10:51
The commit 'ed92d3668ca921270d01055d8d8e01f3fd5187d3' removed the
`setAnsibleEnvVars` function. However, that function is important to
make sure the command line flags `ansible-roles-path` and
`ansible-collections-path` work. These command line flags are set into
environment variables that will be picked up later.
@joelanford joelanford force-pushed the ansible-operator-env branch from 914466c to 1bf89f7 Compare July 27, 2020 14:55
Copy link
Member

@joelanford joelanford 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-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2020
@fabianvf
Copy link
Member

/lgtm

@joelanford joelanford merged commit 3ac3267 into operator-framework:master Jul 27, 2020
@joelanford
Copy link
Member

/cherry-pick v0.19.x

@openshift-cherrypick-robot

@joelanford: #3549 failed to apply on top of branch "v0.19.x":

In response to this:

/cherry-pick v0.19.x

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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
language/ansible Issue is related to an Ansible operator project lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants