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

feat(datasource/docker): Add support for Google Application Default Credentials #23903

Merged
merged 32 commits into from
Sep 21, 2023

Conversation

kvanzuijlen
Copy link
Contributor

@kvanzuijlen kvanzuijlen commented Aug 16, 2023

Changes

This PR adds support for Google Application Default Credentials, which also includes GKE Workload Identity. The benefit of using this is that you don't need a service account key (long-lived token) and is a well documented/best practice solution that doesn't need extra custom work on the end user side.

Context

As said in the section above, this PR alleviates the need for a long lived token and instead introduces an option to configure a well documented solution.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

I'll run these changes on a real repository shortly!

@kvanzuijlen kvanzuijlen changed the title Added support for Google Application Default Credentials feat: Added support for Google Application Default Credentials Aug 16, 2023
lib/modules/datasource/docker/google.ts Outdated Show resolved Hide resolved
lib/modules/datasource/docker/google.ts Outdated Show resolved Hide resolved
Thanks!
@kvanzuijlen
Copy link
Contributor Author

@viceice the code coverage (of course) fails. I don't know how much sense it makes to test my current changes but I can look into writing a test for it if desired. Let me know what you would like me to do!

@kvanzuijlen kvanzuijlen requested a review from viceice August 17, 2023 05:00
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

please run on a real repo

@kvanzuijlen
Copy link
Contributor Author

@viceice I ran it against a real repository and found out that I made a mistake. With the change I made the onboarding PR showed the expected PRs that it would create. Thus proofing (to me) that it works with Google Application Default Credentials, since I didn't set any authorization configs for Artifact Registry.

@kvanzuijlen
Copy link
Contributor Author

@viceice Could you please re-review?

@rarkins rarkins requested a review from viceice August 31, 2023 05:16
@rarkins
Copy link
Collaborator

rarkins commented Sep 1, 2023

Needs deconflicting

@kvanzuijlen
Copy link
Contributor Author

@rarkins done

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

still broken pnpm lockfile

lib/modules/datasource/docker/common.ts Outdated Show resolved Hide resolved
lib/modules/datasource/docker/common.ts Outdated Show resolved Hide resolved
@kvanzuijlen
Copy link
Contributor Author

@viceice Sorry about that, the pnpm lock file rebroke after I fixed it in my earlier commit. It's working now again and I also applied your PR feedback (should've checked it after applying my fixes). Thanks for taking the time to review my PR!

viceice
viceice previously approved these changes Sep 4, 2023
@viceice viceice changed the title feat: Added support for Google Application Default Credentials feat(datasource/docker): Added support for Google Application Default Credentials Sep 4, 2023
@kvanzuijlen kvanzuijlen requested a review from viceice September 19, 2023 17:12
@kvanzuijlen
Copy link
Contributor Author

@viceice is there something I need to change or can this feature be merged? Thank you for your time!

@rarkins
Copy link
Collaborator

rarkins commented Sep 21, 2023

@kvanzuijlen it's best that you don't merge from main any more because that resets the tests to require approval again

lib/modules/datasource/docker/index.spec.ts Outdated Show resolved Hide resolved
lib/modules/datasource/docker/index.spec.ts Outdated Show resolved Hide resolved
lib/modules/datasource/docker/index.spec.ts Outdated Show resolved Hide resolved
@kvanzuijlen kvanzuijlen requested a review from viceice September 21, 2023 12:37
viceice
viceice previously approved these changes Sep 21, 2023
@viceice viceice enabled auto-merge September 21, 2023 14:17
auto-merge was automatically disabled September 21, 2023 14:42

Head branch was pushed to by a user without write access

@kvanzuijlen
Copy link
Contributor Author

kvanzuijlen commented Sep 21, 2023

@viceice I hadn't run prettier after the last PR suggestions, sorry about that.

@viceice viceice enabled auto-merge September 21, 2023 15:02
@viceice viceice added this pull request to the merge queue Sep 21, 2023
Merged via the queue into renovatebot:main with commit 37a0bcf Sep 21, 2023
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 36.102.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kvanzuijlen kvanzuijlen deleted the google-adc-support branch October 7, 2023 17:01
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants