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

Allow azure:// prefix when parsing resource IDs #3616

Merged

Conversation

mboersma
Copy link
Contributor

@mboersma mboersma commented Jun 8, 2023

What type of PR is this?

/kind bug

What this PR does / why we need it:

Handles user-assigned identity resource identifiers starting with an azure:// prefix, which the new ParseResourceID() func doesn't tolerate.

Which issue(s) this PR fixes:

Fixes #3597

Special notes for your reviewer:

  • cherry-pick candidate

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Allow azure:// prefix when parsing resource IDs

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Jun 8, 2023
@k8s-ci-robot k8s-ci-robot requested review from Jont828 and nojnhuh June 8, 2023 16:31
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 8, 2023
@mboersma
Copy link
Contributor Author

mboersma commented Jun 8, 2023

This is a quick fix for that specific issue. Alternately, I could write a wrapper function around ParseResourceID() and have our codebase call that consistently.

@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Patch coverage: 50.00% and project coverage change: -0.27 ⚠️

Comparison is base (6f24d9a) 53.52% compared to head (d61ff8a) 53.26%.

❗ Current head d61ff8a differs from pull request most recent head 38e0c03. Consider uploading reports for the commit 38e0c03 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3616      +/-   ##
==========================================
- Coverage   53.52%   53.26%   -0.27%     
==========================================
  Files         185      185              
  Lines       18544    18435     -109     
==========================================
- Hits         9926     9819     -107     
+ Misses       8076     8074       -2     
  Partials      542      542              
Impacted Files Coverage Δ
azure/scope/machinepool.go 31.17% <0.00%> (ø)
azure/services/natgateways/spec.go 4.08% <0.00%> (ø)
azure/services/virtualmachines/client.go 0.00% <0.00%> (ø)
azure/defaults.go 9.52% <100.00%> (+1.45%) ⬆️
azure/services/scalesetvms/scalesetvms.go 63.41% <100.00%> (ø)

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mboersma mboersma force-pushed the fix-user-assigned-id-format branch from 179864f to 0daf1b0 Compare June 8, 2023 16:45
Copy link
Contributor

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

you are good at doing work while being totally present in a meeting at the same time

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 8, 2023
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

this looks a lot cleaner, thanks!

/lgtm

want to squash?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 6896872a150392f365c0870432894b224b6b3259

@mboersma
Copy link
Contributor Author

mboersma commented Jun 8, 2023

want to squash?

More changes incoming, including webhook validation and unit tests. Sorry for the thrashing, should have been a [WIP].

Edit: actually, I think I'll leave the webhook changes for a subsequent PR. See #3618.

@mboersma mboersma force-pushed the fix-user-assigned-id-format branch from d61ff8a to 6dfe2f0 Compare June 8, 2023 22:05
@mboersma
Copy link
Contributor Author

mboersma commented Jun 8, 2023

/retitle Allow azure:// prefix when parsing resource IDs

@k8s-ci-robot k8s-ci-robot changed the title Allow azure:// prefix in user-assigned ID Allow azure:// prefix when parsing resource IDs Jun 8, 2023
@mboersma
Copy link
Contributor Author

mboersma commented Jun 9, 2023

/cherry-pick release-1.9

@k8s-infra-cherrypick-robot

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

In response to this:

/cherry-pick release-1.9

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.

Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 12, 2023
@mboersma
Copy link
Contributor Author

/override codecov/patch

This change is being penalized for three changed lines of code that were previously not unit tested. I looked at each one, and it's not trivial to write a test harness around those funcs, so I think it's out of scope for this PR to add that. I entered an issue to add unit test coverage: #3622.

@k8s-ci-robot
Copy link
Contributor

@mboersma: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

  • codecov/patch

Only the following failed contexts/checkruns were expected:

  • EasyCLA
  • deploy/netlify
  • pull-cluster-api-provider-azure-apidiff
  • pull-cluster-api-provider-azure-build
  • pull-cluster-api-provider-azure-capi-e2e
  • pull-cluster-api-provider-azure-ci-entrypoint
  • pull-cluster-api-provider-azure-conformance
  • pull-cluster-api-provider-azure-conformance-with-ci-artifacts
  • pull-cluster-api-provider-azure-e2e
  • pull-cluster-api-provider-azure-e2e-aks
  • pull-cluster-api-provider-azure-test
  • pull-cluster-api-provider-azure-verify
  • pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts
  • tide

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

In response to this:

/override codecov/patch

This change is being penalized for three changed lines of code that were previously not unit tested. I looked at each one, and it's not trivial to write a test harness around those funcs, so I think it's out of scope for this PR to add that. I entered an issue to add unit test coverage: #3622.

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.

@mboersma
Copy link
Contributor Author

I'm confused; I thought we had configured prow so codecov was informational, not blocking. Yet prow is saying it won't merge because codecov has not succeeded.

@CecileRobertMichon
Copy link
Contributor

CecileRobertMichon commented Jun 12, 2023

@mboersma we had, however @willie-yao recently got the k8s GitHub admins to add the codecov app to the CAPZ repo, I wonder if that is what caused a change

Edit: probably, we have both codecov/project and codecov/app now

@mboersma
Copy link
Contributor Author

mboersma commented Jun 12, 2023

Can we do "override/tide"? 🔨

@mboersma
Copy link
Contributor Author

/override tide

@k8s-ci-robot
Copy link
Contributor

@mboersma: Overrode contexts on behalf of mboersma: tide

In response to this:

/override tide

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.

@CecileRobertMichon
Copy link
Contributor

/override codecov/patch

@k8s-ci-robot
Copy link
Contributor

@CecileRobertMichon: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

  • codecov/patch

Only the following failed contexts/checkruns were expected:

  • EasyCLA
  • deploy/netlify
  • pull-cluster-api-provider-azure-apidiff
  • pull-cluster-api-provider-azure-build
  • pull-cluster-api-provider-azure-capi-e2e
  • pull-cluster-api-provider-azure-ci-entrypoint
  • pull-cluster-api-provider-azure-conformance
  • pull-cluster-api-provider-azure-conformance-with-ci-artifacts
  • pull-cluster-api-provider-azure-e2e
  • pull-cluster-api-provider-azure-e2e-aks
  • pull-cluster-api-provider-azure-test
  • pull-cluster-api-provider-azure-verify
  • pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts
  • tide

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

In response to this:

/override codecov/patch

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.

@mboersma mboersma force-pushed the fix-user-assigned-id-format branch from 6dfe2f0 to 38e0c03 Compare June 13, 2023 18:24
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2023
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 5b09606fd88448983e556e175338821fbafa1cfc

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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

@k8s-infra-cherrypick-robot

@mboersma: new pull request created: #3626

In response to this:

/cherry-pick release-1.9

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.

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

userAssignedIdentities providerID value format unexpectedly changed in CAPZ v1.9.0
5 participants