Skip to content

CORS-2472: Expose Azure useImageGallery parameter in the MachineSets() call #6737

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.

Sign up for GitHub

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

Jump to bottom
Merged
openshift-merge-robot merged 1 commit into from
Jan 9, 2023
Merged

CORS-2472: Expose Azure useImageGallery parameter in the MachineSets() call #6737

openshift-merge-robot merged 1 commit into from
Jan 9, 2023

Conversation

@dlom
Copy link
Contributor

@dlom dlom commented Jan 4, 2023

As a followup to #6694, Hive needs to be able to select whether to use the image gallery setting or not. This change just forwards the useImageGallery further up the chain, and also removes the unused rhcosVersion parameter.

/cc @patrickdillon

@openshift-ci openshift-ci bot requested a review from patrickdillon January 4, 2023 18:28
@2uasimojo
Copy link
Member

2uasimojo commented Jan 4, 2023

This looks good to me. for missing it the first time around.

@dlom
Copy link
Contributor Author

dlom commented Jan 4, 2023

/assign @patrickdillon

@r4f4
Copy link
Contributor

r4f4 commented Jan 4, 2023

It would be nice if the commit message was more descriptive as suggested in our contributing guidelines: https://github.com/openshift/installer/blob/master/CONTRIBUTING.md#commit-message-format

@r4f4
Copy link
Contributor

r4f4 commented Jan 4, 2023

golint is failing with

 pkg/asset/machines/worker.go:415:58: G601: Implicit memory aliasing in for loop. (gosec)
			sets, err := azure.MachineSets(clusterID.InfraID, ic, &pool, string(*rhcosImage), "worker", workerUserDataSecretName, capabilities, useImageGallery)

Even though this was not introduced by this PR, the golint tool is setup in such a way it'll report existing problems when new changes touch them. There are 2 ways to appease the linter here:

  1. At the beginning of the loop we do the following so the issue is fixed for every place that uses &pool in this loop
	for _, pool := range ic.Compute {
            pool := pool
  1. We ignore the warning since we know it's fine in this case
sets, err := azure.MachineSets(clusterID.InfraID, ic, &pool, string(*rhcosImage), "worker", workerUserDataSecretName, capabilities, useImageGallery) //nolint:gosec

@dlom
Azure MachineSets: Expose useImageGallery to consumers
92b521e 
This allows end-users of the MachineSets() function to choose whether
they want to use the image gallery or not (<=4.11 vs >=4.12).
Additionally, the unused parameter rhcosVerison has been removed, and
a variable has been shadowed to appease the golint CI check.

See openshift#6694 for more details concerning why this change is necessary.
@dlom dlom force-pushed the OCPBUGS-4809-followup branch from fbff8d2 to 92b521e Compare January 5, 2023 00:14
@dlom
Copy link
Contributor Author

dlom commented Jan 5, 2023

@r4f4 hopefully this appeases the golint CI... thanks for the review

@r4f4
Copy link
Contributor

r4f4 commented Jan 5, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 5, 2023
@r4f4 r4f4 removed their assignment Jan 5, 2023
@patrickdillon
Copy link
Contributor

patrickdillon commented Jan 6, 2023

/approve
/cherry-pick release-4.12
/label backport-risk-assessed

@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Jan 6, 2023

@patrickdillon: once the present PR merges, I will cherry-pick it on top of release-4.12 in a new PR and assign it to you.

Details

In response to this:

/approve
/cherry-pick release-4.12
/label backport-risk-assessed

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.

@openshift-ci openshift-ci bot added the backport-risk-assessed Indicates a PR to a release branch has been evaluated and considered safe to accept. label Jan 6, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 6, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 6, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 6, 2023

/retest-required

Remaining retests: 0 against base HEAD f2cc8be and 2 for PR HEAD 92b521e in total

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 7, 2023

/retest-required

Remaining retests: 0 against base HEAD e284448 and 1 for PR HEAD 92b521e in total

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 9, 2023

/retest-required

Remaining retests: 0 against base HEAD a3e3714 and 0 for PR HEAD 92b521e in total

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 9, 2023 •
edited
Loading

@dlom: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-libvirt 92b521e link false /test e2e-libvirt
ci/prow/okd-e2e-aws-ovn-upgrade 92b521e link false /test okd-e2e-aws-ovn-upgrade
ci/prow/e2e-azurestack 92b521e link false /test e2e-azurestack
ci/prow/okd-scos-e2e-aws-upgrade 92b521e link false /test okd-scos-e2e-aws-upgrade
ci/prow/e2e-aws-ovn-disruptive 92b521e link false /test e2e-aws-ovn-disruptive
ci/prow/e2e-ibmcloud-ovn 92b521e link false /test e2e-ibmcloud-ovn
ci/prow/e2e-openstack 92b521e link false /test e2e-openstack
ci/prow/e2e-aws-ovn-upgrade 92b521e link false /test e2e-aws-ovn-upgrade

Full PR test history. Your PR dashboard.

Details

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.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 9, 2023

/hold

Revision 92b521e was retested 3 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2023
@patrickdillon
Copy link
Contributor

patrickdillon commented Jan 9, 2023

/hold cancel
/skip

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2023
@patrickdillon
Copy link
Contributor

patrickdillon commented Jan 9, 2023

/override ci/prow/e2e-gcp-ovn

1 similar comment
@patrickdillon
Copy link
Contributor

patrickdillon commented Jan 9, 2023

/override ci/prow/e2e-gcp-ovn

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 9, 2023

@patrickdillon: Overrode contexts on behalf of patrickdillon: ci/prow/e2e-gcp-ovn

Details

In response to this:

/override ci/prow/e2e-gcp-ovn

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.

@openshift-merge-robot openshift-merge-robot merged commit ed8b918 into openshift:master Jan 9, 2023
@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Jan 9, 2023
Merged
@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Jan 9, 2023

@patrickdillon: new pull request created: #6753

Details

In response to this:

/approve
/cherry-pick release-4.12
/label backport-risk-assessed

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.

@dlom
Copy link
Contributor Author

dlom commented Jan 9, 2023

🎉 thanks everyone

@dlom
Copy link
Contributor Author

dlom commented Jan 11, 2023 •
edited by openshift-ci bot
Loading

/retitle CORS-2472: Expose Azure useImageGallery parameter in the MachineSets() call

@openshift-ci openshift-ci bot changed the title Expose Azure useImageGallery parameter in the MachineSets() call CORS-2472: Expose Azure useImageGallery parameter in the MachineSets() call Jan 11, 2023
@dlom
Copy link
Contributor Author

dlom commented Jan 11, 2023

/bugzilla refresh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@patrickdillon patrickdillon Awaiting requested review from patrickdillon

Assignees

@patrickdillon patrickdillon

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. backport-risk-assessed Indicates a PR to a release branch has been evaluated and considered safe to accept. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@dlom @2uasimojo @r4f4 @patrickdillon @openshift-cherrypick-robot @openshift-ci-robot @openshift-merge-robot