Skip to content

stop considering Mint mode as supported on Azure #5496

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
Closed
joelddiaz wants to merge 1 commit into from
Closed

stop considering Mint mode as supported on Azure #5496

joelddiaz wants to merge 1 commit into from

Conversation

@joelddiaz
Copy link

@joelddiaz joelddiaz commented Dec 17, 2021

@joelddiaz
Copy link
Author

joelddiaz commented Dec 17, 2021

/hold wait on openshift/cloud-credential-operator#433

@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 Dec 17, 2021
@patrickdillon
Copy link
Contributor

patrickdillon commented Dec 20, 2021

/test e2e-azure

patrickdillon
patrickdillon reviewed Dec 20, 2021
View reviewed changes
pkg/types/validation/installconfig.go Outdated
Copy link
Contributor

@patrickdillon patrickdillon Dec 20, 2021

Choose a reason for hiding this comment

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

This seems reasonable to me. #5325 is a start to clean up some loose ends in how the installer handles credential modes (I think we want to move toward choosing defaults rather than allowing credentialmode to be ""). #5325 would need to handle this case as well, where we have two modes.

Copy link
Contributor

@staebler staebler Feb 1, 2022

Choose a reason for hiding this comment

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

The issue with choosing a default instead of an empty string is that they mean different things. When an explicit selection is made, it (1) tells the cloud-credential-operator to use that mode and (2) skips permissions validation. When an empty string is used, it tells the cloud-credential-operator to inspect the credentials provided and make an opinionated decision about which mode to use based upon what is allowed with the credentials.

Copy link
Author

@joelddiaz joelddiaz Feb 1, 2022

Choose a reason for hiding this comment

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

Of course, now with Azure there is only Passthrough. So no more intelligent querying of the creds.

@staebler
Copy link
Contributor

staebler commented Feb 1, 2022

/hold wait on openshift/cloud-credential-operator#433

@joelddiaz Are we ready to remove the hold on this PR?

@staebler
Copy link
Contributor

staebler commented Feb 1, 2022

/lgtm
/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 1, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler

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 Feb 1, 2022
@staebler
Copy link
Contributor

staebler commented Feb 1, 2022

Changes need to be made to

installer/pkg/types/installconfig.go

Line 146 in f6ea846

// Azure: "Mint", "Passthrough", "Manual"
as well.

/lgtm cancel

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2022
@joelddiaz
Copy link
Author

joelddiaz commented Feb 1, 2022

/unhold

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 1, 2022

New changes are detected. LGTM label has been removed.

@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 Feb 1, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 1, 2022

@joelddiaz: 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-alibaba 65831b75cb3dc8f2b9f597f4ccc6d3f61fc23ceb link true /test e2e-alibaba
ci/prow/e2e-crc 0d828c2 link false /test e2e-crc
ci/prow/e2e-aws-upgrade 0d828c2 link true /test e2e-aws-upgrade
ci/prow/okd-e2e-aws-upgrade 0d828c2 link false /test okd-e2e-aws-upgrade
ci/prow/unit 0d828c2 link true /test unit
ci/prow/e2e-aws-single-node 0d828c2 link false /test e2e-aws-single-node
ci/prow/e2e-aws-workers-rhel7 0d828c2 link false /test e2e-aws-workers-rhel7
ci/prow/e2e-aws-workers-rhel8 0d828c2 link false /test e2e-aws-workers-rhel8
ci/prow/e2e-azure-upi 0d828c2 link false /test e2e-azure-upi
ci/prow/e2e-ibmcloud 0d828c2 link false /test e2e-ibmcloud
ci/prow/okd-unit 0d828c2 link true /test okd-unit
ci/prow/e2e-openstack-kuryr 0d828c2 link false /test e2e-openstack-kuryr
ci/prow/e2e-aws-fips 0d828c2 link false /test e2e-aws-fips

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.

@r4f4 r4f4 mentioned this pull request Mar 11, 2022
Merged
@patrickdillon
Copy link
Contributor

patrickdillon commented Mar 17, 2022

/close
in favor of #5699

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 17, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 17, 2022

@joelddiaz: PR needs rebase.

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 17, 2022

@patrickdillon: Closed this PR.

Details

In response to this:

/close
in favor of #5699

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 closed this Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@patrickdillon patrickdillon patrickdillon left review comments

@jhixson74 jhixson74 Awaiting requested review from jhixson74

@squidboylan squidboylan Awaiting requested review from squidboylan

+1 more reviewer

@staebler staebler staebler left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@staebler staebler

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@joelddiaz @patrickdillon @staebler