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

Always use passed ConnectConfig when creating clients #157

Merged
merged 2 commits into from
Jul 8, 2024

Conversation

AndreasBurger
Copy link
Member

@AndreasBurger AndreasBurger commented Jun 26, 2024

What this PR does / why we need it:
This PR is a follow-up to #148. Due to an oversight, the previous change caused only the authentication to connect to the intended cloud.

I've obtained some credentials for other Instances in the meantime and was able to verify that with these changes, creating Machines on other Providers can succeed.

Special notes for your reviewer:
I've gone ahead and made the passed ConnectConfig the source of truth for the connection-options. One could think about merging it with the factory-provided options, but since these are currently always the default options I've decided not to do this for now.

Release note:

Fixed a bug that prevented everything but authentication from connecting to non-public Azure Cloud instances.

@AndreasBurger AndreasBurger requested review from a team as code owners June 26, 2024 15:18
@gardener-robot gardener-robot added the needs/review Needs review label Jun 26, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 26, 2024
@gardener-robot gardener-robot added the size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) label Jun 26, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 26, 2024
Copy link
Contributor

@rishabh-11 rishabh-11 left a comment

Choose a reason for hiding this comment

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

Can you remove clientOptions from the defaultFactory? It's not required anymore as we will be passing those options in the connectConfig.

@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Jun 27, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 28, 2024
Copy link
Contributor

@rishabh-11 rishabh-11 left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/review Needs review labels Jul 3, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 3, 2024
@AndreasBurger AndreasBurger merged commit dd3ac4f into gardener:master Jul 8, 2024
7 checks passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Jul 8, 2024
rishabh-11 pushed a commit to rishabh-11/machine-controller-manager-provider-azure that referenced this pull request Jul 18, 2024
Always use passed ConnectConfig when creating clients
rishabh-11 added a commit that referenced this pull request Jul 18, 2024
Always use passed ConnectConfig when creating clients

Co-authored-by: Andreas Burger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants