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

Update setup-go action for latest version #55

Merged
merged 2 commits into from
Jun 3, 2024
Merged

Conversation

katiem0
Copy link
Contributor

@katiem0 katiem0 commented May 31, 2024

Existing actions/setup-go@v3 usage is referring to legacy/deprecated node16 in the actions:

runs:
  using: 'node16'
  main: 'dist/setup/index.js'
  post: 'dist/cache-save/index.js'
  post-if: success()

This PR updates the setup-go action to use latest version.

Confirmed stripped down logic is successful in GitHub CLI extension: katiem0/gh-collaborators

No identified breaking changes are introduced between v3 and v5.

@katiem0 katiem0 requested a review from a team as a code owner May 31, 2024 01:08
@katiem0 katiem0 requested a review from williammartin May 31, 2024 01:08
andyfeller
andyfeller previously approved these changes May 31, 2024
Copy link
Contributor

@andyfeller andyfeller left a comment

Choose a reason for hiding this comment

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

Thank you for improving the GitHub CLI and supporting extension authors, @katiem0! 🫶

Does it make sense to bubble up new inputs to allow users to control whether and how caching is used?

☝️ is my only concern as it is not a pure binary off/on as it also involves cache-dependency-path too. 🤔

@andyfeller andyfeller self-requested a review May 31, 2024 16:00
@andyfeller andyfeller dismissed their stale review May 31, 2024 16:01

Accidentally selected approve when I intended to leave a comment

@katiem0
Copy link
Contributor Author

katiem0 commented May 31, 2024

@andyfeller Can definitely add to it! Wasn't sure if we'd want to add more features - currently the action fails because of the node16 usage, so wanted to get it to a working state asap. Happy to add it in if we want that, but maybe a tech-debt/new issue for it to ensure we're accounting for the new input scenarios? 😄

@andyfeller
Copy link
Contributor

@andyfeller Can definitely add to it! Wasn't sure if we'd want to add more features - currently the action fails because of the node16 usage, so wanted to get it to a working state asap. Happy to add it in if we want that, but maybe a tech-debt/new issue for it to ensure we're accounting for the new input scenarios? 😄

Yeah, I can buy into deferring that until someone has a problem with it and we know more. At the very least, is it worth updating the README to call out that Go dependencies are cached?

@katiem0
Copy link
Contributor Author

katiem0 commented Jun 3, 2024

@andyfeller README now has NOTE indicating the change and pointing to the action version from setup-go.

@andyfeller andyfeller merged commit ee3e4e5 into cli:trunk Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants