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

[release-3.11] Call version.yml before repos.yml to ensure openshift_version is set #10742

Merged

Conversation

tzumainn
Copy link
Contributor

repos.yml requires openshift_version to be set if RHEL repositories are to be enabled. Calling version.yml before repos.yml ensures openshift_version is set.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 21, 2018
@tzumainn
Copy link
Contributor Author

@sdodson , let me know if there was a reason why version.yml can't be called before; I'm not excessively familiar with the openshift-side playbooks.

@tzumainn tzumainn changed the title Call version.yml before repos.yml to ensure openshift_version is set [release-3.11] Call version.yml before repos.yml to ensure openshift_version is set Nov 26, 2018
@tzumainn
Copy link
Contributor Author

tzumainn commented Dec 3, 2018

/retest

@luis5tb
Copy link
Contributor

luis5tb commented Dec 3, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2018
@tzumainn
Copy link
Contributor Author

tzumainn commented Dec 3, 2018

@vrutkovs Would it be possible for you to take a look, or suggest someone... ?

@vrutkovs
Copy link
Member

vrutkovs commented Dec 4, 2018

/cc @michaelgugino

Copy link
Contributor

@michaelgugino michaelgugino left a comment

Choose a reason for hiding this comment

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

/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 Dec 4, 2018
@@ -22,6 +22,8 @@

- import_playbook: basic_facts.yml

- import_playbook: version.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

This change conflicts with the need to run cluster_facts.yml in order to set current version to feed into version role.

Here's where we are now: Our openshift_repos role has become a "openshift and rhel-server repos" role which it was never intended to be. Thus, there is a requirement to run repos before base_packages for some of our users (openstack, possibly aws provisioning plays).

What we can do instead:

In 3.11, we put a default value for openshift_release in the openshift_version role. We can pull openshift_version in as a meta-dependency on the repos role, and use variable openshift_release in place of variable openshift_version in the repos role.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michaelgugino Thanks for the review! What default value would you suggest for openshift_release? Would you like me to attempt this change, or would you prefer someone with more experience with the repos roles?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tzumainn If you include openshift_version role as a dependency in roles/openshift_repos/meta/main.yml, you won't need to set openshift_release because we have a default value for it defined in the openshift_version role.

@michaelgugino
Copy link
Contributor

/lgtm cancel

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2018
@tzumainn tzumainn force-pushed the init-version-before-repos-3.11 branch from e2f81b6 to 0c50652 Compare December 4, 2018 18:57
@tzumainn
Copy link
Contributor Author

tzumainn commented Dec 4, 2018

@michaelgugino thanks! I think I understand. I've updated the PR; is this what you meant?

@tzumainn
Copy link
Contributor Author

tzumainn commented Dec 4, 2018

/retest

Copy link
Contributor

@michaelgugino michaelgugino left a comment

Choose a reason for hiding this comment

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

This is not what I meant. I'm not sure how else to describe what I meant; I can get a patch out tomorrow.

@tzumainn
Copy link
Contributor Author

tzumainn commented Dec 4, 2018

@michaelgugino I see what you mean now, but leads me to a question: roles/openshift_version/tasks/main.yml is empty; instead it's first_master.yml that sets the relevant parameters. I couldn't find a way to specify that within a role dependency; is there some syntax that would help here?

@michaelgugino
Copy link
Contributor

@michaelgugino I see what you mean now, but leads me to a question: roles/openshift_version/tasks/main.yml is empty; instead it's first_master.yml that sets the relevant parameters. I couldn't find a way to specify that within a role dependency; is there some syntax that would help here?

@tzumainn essentially what I'm suggesting would be to bring the openshift_version role's defaults (defaults/main.yml) into scope. I don't intend for any tasks to actually run, the only value I'm interested in is openshift_release. We can modify openshift_repos role to use openshift_release variable instead of openshift_version variable.

@tzumainn tzumainn force-pushed the init-version-before-repos-3.11 branch from 0c50652 to d95ca4f Compare December 5, 2018 15:14
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 5, 2018
@tzumainn
Copy link
Contributor Author

tzumainn commented Dec 5, 2018

@michaelgugino Thanks for the clarification. I updated the PR; is this what you're suggesting?

@tzumainn
Copy link
Contributor Author

tzumainn commented Dec 5, 2018

/retest

@tzumainn
Copy link
Contributor Author

tzumainn commented Dec 5, 2018

/test e2e-aws

Copy link
Contributor

@michaelgugino michaelgugino 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 Dec 6, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: luis5tb, michaelgugino, tzumainn

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 Dec 6, 2018
@tzumainn
Copy link
Contributor Author

tzumainn commented Dec 6, 2018

/test e2e-aws

@tzumainn
Copy link
Contributor Author

tzumainn commented Dec 6, 2018

@michaelgugino Thanks for the /lgtm! Does the PR also need the hold removed?

@michaelgugino
Copy link
Contributor

/hold cancel

@michaelgugino
Copy link
Contributor

/test e2e-aws

4 similar comments
@michaelgugino
Copy link
Contributor

/test e2e-aws

@tzumainn
Copy link
Contributor Author

tzumainn commented Dec 6, 2018

/test e2e-aws

@tzumainn
Copy link
Contributor Author

tzumainn commented Dec 7, 2018

/test e2e-aws

@michaelgugino
Copy link
Contributor

/test e2e-aws

@michaelgugino michaelgugino removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 7, 2018
@tzumainn
Copy link
Contributor Author

tzumainn commented Dec 7, 2018

/test e2e-aws

3 similar comments
@tzumainn
Copy link
Contributor Author

tzumainn commented Dec 9, 2018

/test e2e-aws

@tzumainn
Copy link
Contributor Author

/test e2e-aws

@tzumainn
Copy link
Contributor Author

/test e2e-aws

@sdodson
Copy link
Member

sdodson commented Dec 11, 2018

/refresh

@openshift-ci-robot
Copy link

@tzumainn: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-gcp d95ca4f link /test e2e-gcp

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@sdodson
Copy link
Member

sdodson commented Dec 11, 2018

GCP quota problems.

@sdodson sdodson merged commit 7128178 into openshift:release-3.11 Dec 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants