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

chore(ci): Upgrade workflows to non-deprecated runtimes #719

Merged
merged 5 commits into from
Apr 11, 2023

Conversation

dev-slatto
Copy link
Contributor

Description of the issue

Hi! 👋🏻

Several of the Github Actions in the repo is using deprecated runtimes for its workflows.

Description of changes

How does this change address the problem?
I've upgraded the deprecated actions to versions that uses non-deprecated runtimes.

Note that setup-go@v4 will try to enable caching unless the cache input is explicitly set to false. I'll be more than happy to make it false if wanted as I see that you already do a lot of cache in the different pipelines.

Open to feedback if there are any other changes wanted done regarding Github Actions 😄

License

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Tests

Describe what tests you have done.

Requirements

Before commit the code, please do the following steps.

  1. Run make fmt and make fmt-sh
  2. Run make linter

@dev-slatto dev-slatto requested a review from a team as a code owner March 15, 2023 07:26
@khanhntd
Copy link
Contributor

Hi @dev-slatto ,
Thanks for the contribution and your PR contains all the PR from dependandot (e.g stale) so thanks for that, this would help us a lot though.

@dev-slatto
Copy link
Contributor Author

No worries! For the sake of it and to clean up the PR overview I'll add here what this one closes 😄

Closes #709 #708 #707 #706 #705

@khanhntd
Copy link
Contributor

Hi @dev-slatto ,
The PR look good to me and there are no concern over it (but no PM is needed for the changes too so good news) when looking the breaking changes of all the Github Action. (including the slate action). I agree with you regarding adding cache in setup@v4. Even though I saw its mentioned regarding use the key with the hash of go sum, I have not seen whether it can applied to cache with different OS. Therefore, if you upgrading it, then we can use the PR workflow to validate it.

No worries! For the sake of it and to clean up the PR overview I'll add here what this one closes 😄

It will automatically close the PR with dependandot when its checking the Github workflow Github action version but its good to link it too

@codecov-commenter
Copy link

codecov-commenter commented Mar 17, 2023

Codecov Report

Merging #719 (96cbd8f) into main (32d78d1) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #719      +/-   ##
==========================================
- Coverage   56.98%   56.95%   -0.03%     
==========================================
  Files         375      375              
  Lines       17878    17878              
==========================================
- Hits        10187    10182       -5     
- Misses       7092     7095       +3     
- Partials      599      601       +2     

see 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@dev-slatto
Copy link
Contributor Author

Cool stuff @khanhntd! Do you need anything else from me to this PR or does it look good? 😄
I see that one test is failing, but it looks like it's related to some bad creds in a EC2 rather than the changes in this PR.

@khanhntd
Copy link
Contributor

Yes @dev-slatto , can you help us trying setup-go@v4 with cache turning on and deleting the actions/cache@v3 to see if the cache from setup-go@v4 will be good with various OS? Agree with the mac-os are credentials thing so you can ignore it, we have aware of the issue and make an attempt to fix it.

@dev-slatto
Copy link
Contributor Author

Hey again @khanhntd 👋🏻
Latest commit should let the setup-go@v4 workflow handle dependency cache in the workflows.

@khanhntd
Copy link
Contributor

Re-run the workflow to confirm cache is working. In the mean time, you can also fix the merge conflicts.

@khanhntd
Copy link
Contributor

For all cached hmm

Warning: Restore cache failed: Some specified paths were not resolved, unable to cache dependencies.

@dev-slatto
Copy link
Contributor Author

@khanhntd After what I've read it looks like the cache-dependency-path actually needs to reference the go.sum file generated for the cache to work. Do we know all of these paths or would we like to then keep the old cache that supports referencing paths instead of the sum files?

@khanhntd
Copy link
Contributor

Do we know all of these paths or would we like to then keep the old cache that supports referencing paths instead of the sum files?

We might not know. There are no examples in Github that's shows anyone is using it so its being a blind spot even for us here. However, if that's the case, we can go with the path turn off the cache and use old cache that referencing paths

@dev-slatto
Copy link
Contributor Author

@khanhntd then the conflict should be resolved and the dedicated cache step should be implemented again 😄

Copy link
Contributor

@khanhntd khanhntd left a comment

Choose a reason for hiding this comment

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

Only 1 small comment before approving it !!!!! Thanks for spending your effort on this !!!

.github/workflows/clean-aws-resources.yml Outdated Show resolved Hide resolved
@dev-slatto
Copy link
Contributor Author

All done @khanhntd! 😄
Thanks for taking the time and giving the feedback!

Copy link
Contributor

@khanhntd khanhntd left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution !!! Will get another person to review your PR !!

@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2023

This PR was marked stale due to lack of activity.

@github-actions github-actions bot added the Stale label Apr 8, 2023
@williazz williazz merged commit 98c12ac into aws:main Apr 11, 2023
@williazz
Copy link
Member

thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants