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

Use the right option for the cluster client #3368

Closed
wants to merge 1 commit into from

Conversation

nwnt
Copy link
Contributor

@nwnt nwnt commented Jan 24, 2024

Which issue this PR addresses:

Fixes: ARO-3315

What this PR does / why we need it:

Make sure the az identity client use the right option. At the moment it's set with nil.

Test plan for issue:

E2E

Is there any documentation that needs to be updated for this PR?

No

Copy link
Collaborator

@mociarain mociarain left a comment

Choose a reason for hiding this comment

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

Small suggestion on it as it. On a general note I think we could consider breaking this function up into a few parts. It's quite long and essentially there are 2 parts labelled // X Validation that could be broken out easily.

What do you think?

@mociarain
Copy link
Collaborator

mociarain commented Jan 24, 2024

Also does this have any bearing for the linked epic?

@nwnt
Copy link
Contributor Author

nwnt commented Jan 24, 2024

ARO-2248 is the work item to remove that after all the releases have been completed. As for the refactoring suggestion, I think it's better to keep or track that as a separate task as this one is aimed to fix the apparent bug. IMHO, that's going to be a non-trivial task because there's not a single unit test that validates this function. Probably that's a required step prior to any refactoring efforts.

@cadenmarchese
Copy link
Collaborator

/azp run e2e

Copy link

No pipelines are associated with this pull request.

@cadenmarchese
Copy link
Collaborator

@nwnt @mociarain as discussed in review today, I think we could handle the refactor in another PR since this is a bugfix.

@mociarain
Copy link
Collaborator

@nwnt @mociarain as discussed in review today, I think we could handle the refactor in another PR since this is a bugfix.

Np. Happy to do the refactor myself to get the practice in. Just let me know :)

@nwnt nwnt added the next-release To be included in the next RP release rollout label Jan 25, 2024
@nwnt
Copy link
Contributor Author

nwnt commented Jan 25, 2024

/azp run e2e

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@cadenmarchese
Copy link
Collaborator

/azp run e2e

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@github-actions github-actions bot added needs-rebase branch needs a rebase and removed ready-for-review labels Feb 2, 2024
Copy link

github-actions bot commented Feb 2, 2024

Please rebase pull request.

@nwnt
Copy link
Contributor Author

nwnt commented Feb 6, 2024

No longer need this PR as the same fix is applied in #3335.

@nwnt nwnt closed this Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase branch needs a rebase next-release To be included in the next RP release rollout
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants