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

Notify the apb what cluster it's running on with extravars #577

Merged
merged 3 commits into from
Dec 12, 2017

Conversation

rthallisey
Copy link
Contributor

Describe what this PR does and why we need it:
Send the variable 'cluster' as an extravar to the apb so it can identify which playbook to run based on the runtime.

Changes proposed in this pull request

  • Pass cluster=<kubernetes/openshift> as an extravar to the apb on creation

depends-on: ansibleplaybookbundle/postgresql-apb#23
depends-on: ansibleplaybookbundle/mediawiki-apb#17

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 6, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 1042494 on rthallisey:extravars into ** on openshift:master**.

Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

LGTM. I'll change my vote when the test(s) are implemented.

@@ -16,3 +16,8 @@ func (f fakeProvider) DestroySandbox(podName string, namespace string, targets [
//TODO: Write tests for DestroySandbox using the fake kubernetes client
return
}

func (f fakeProvider) GetRuntime() string {
Copy link
Member

Choose a reason for hiding this comment

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

I would like to see tests implemented for this change before giving the 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deal

@@ -198,6 +198,8 @@ func createExtraVars(context *Context, parameters *Parameters) (string, error) {
if context != nil {
paramsCopy["namespace"] = context.Namespace
}

paramsCopy["cluster"] = runtime.Provider.GetRuntime()
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if I believe this should be an extra variable or an environment variable that APBs should load and have default values for...

Copy link
Member

Choose a reason for hiding this comment

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

I'm curious the reason that you have this as an argument instead of an environment variable to the APB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 for curiosity David. The reason I went with extravars is because it will overwrite the value of cluster right from the command line. Then we don't have to 1) add a default environment var to apb-base, 2) I don't need to rely on the user setting CLUSTER=kubernetes to run the playbooks locally, and 3) I can default the var right in the playbook:

vars:
     cluster: kubernetes
docker run docker.io/ansibleplaybookbundle/mediawiki-apb provision --extra-vars cluster=kubernetes

ansible-playbook playbooks/provision.yaml --extra-vars "cluster=kubernetes"

Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

CONDACK, look at a test like @djzager said. Other than that this is acceptable.

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 7, 2017
Ryan Hallisey added 3 commits December 7, 2017 15:00
Send the variable cluster as an extravar to the apb.
The executor file needs to be re done so tests can be created
for each function.
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 7, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 07650ca on rthallisey:extravars into ** on openshift:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 07650ca on rthallisey:extravars into ** on openshift:master**.

@rthallisey
Copy link
Contributor Author

@djzager I added the test in. Let me know if you have any more concerns.

Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for adding tests.

@rthallisey rthallisey merged commit 02dabc4 into openshift:master Dec 12, 2017
@rthallisey rthallisey deleted the extravars branch December 12, 2017 13:53
jianzhangbjz pushed a commit to jianzhangbjz/ansible-service-broker that referenced this pull request May 17, 2018
…#577)

* Notify the apb what cluster it's running on with extravars

Send the variable cluster as an extravar to the apb.

* Add tests for GetRuntime

* Remove extravars test since it uses the runtime pkg

The executor file needs to be re done so tests can be created
for each function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants