ratify icon indicating copy to clipboard operation
ratify copied to clipboard

chore: migrate azure-sdk-for-go/containerregistry to the latest release

Open shahramk64 opened this issue 1 year ago • 1 comments

Description

What this PR does / why we need it:

auth provider currently uses an old preview version of azure sdk for go. With the latest release of the sdk, the necessary API to exchange the AAD access token for an ACR refresh token is exposed and we can migrate to this latest release.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):

Fixes #959

Type of change

Please delete options that are not relevant.

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] Helm Chart Change (any edit/addition/update that is necessary for changes merged to the main branch)
  • [ ] This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

  • [ ] Test A
  • [ ] Test B

Checklist:

  • [ ] Does the affected code have corresponding tests?
  • [ ] Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
  • [ ] Does this introduce breaking changes that would require an announcement or bumping the major version?
  • [ ] Do all new files have appropriate license header?

Post Merge Requirements

  • [ ] MAINTAINERS: manually trigger the "Publish Package" workflow after merging any PR that indicates Helm Chart Change

shahramk64 avatar Sep 26 '24 01:09 shahramk64

Hi @shahramk64 , thanks for the PR. please link to the AKS run in your fork once this is ready for review. thanks!

susanshi avatar Sep 27 '24 00:09 susanshi

@akashsinghal @susanshi I added unit tests to azureworkloadidentity_test.go. I would appreciate it if you could have a look at it. Based on what I understood, to mock the exported functions in azcontainerregistry, I needed to use dependency injection, so I refactored azureworkloadidentity.go for that. If this is the right way to write the unit tests, I'll follow the same pattern for azureidentity.go and will try to bring the coverage up to 80%

shahramk64 avatar Sep 29 '24 09:09 shahramk64

@akashsinghal @susanshi I added unit tests to azureworkloadidentity_test.go. I would appreciate it if you could have a look at it. Based on what I understood, to mock the exported functions in azcontainerregistry, I needed to use dependency injection, so I refactored azureworkloadidentity.go for that. If this is the right way to write the unit tests, I'll follow the same pattern for azureidentity.go and will try to bring the coverage up to 80%

Thanks @shahramk64. I think this mock approach makes sense if the underlying azure client cannot be mocked

akashsinghal avatar Sep 30 '24 17:09 akashsinghal

Please validate e2e AKS result before merging.

susanshi avatar Oct 15 '24 00:10 susanshi

LGTM. Thanks for working on this. Pending AKS e2e validation.

akashsinghal avatar Oct 15 '24 01:10 akashsinghal

Here is a link to a successfull e2e AKS run on my forked repo: https://github.com/shahramk64/forked_ratify/actions/runs/11340226960/job/31587804097?pr=3

shahramk64 avatar Oct 16 '24 00:10 shahramk64

Here is a link to the latest e2e AKS run on my forked repo: https://github.com/shahramk64/forked_ratify/actions/runs/11451048602/job/31860970244?pr=3

shahramk64 avatar Oct 22 '24 02:10 shahramk64