-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
Refactor + Modernise Actions #4082
Conversation
c3fbebf
to
86ff4d1
Compare
This comment was marked as resolved.
This comment was marked as resolved.
40b7cb4
to
9107e28
Compare
9107e28
to
2bf9c09
Compare
This breaks apart our build/test workflows to make them reusable. With the aim of being able to reduce deplication, improve maintainability, and efficiency of how the workflows run.
2bf9c09
to
13bd019
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a couple of things that look like they need more attention.
But I love the overall approach here, and I'm really looking forward to using these nicely refactored workflows!
Awesome! I note the dotnet action was removed, now I'm aware it's a no-op, because they are available by default. But that may not always be the case, and we are setting version 7 specifically. So it wasn't an accident that they appeared again. I guess if it stops working in the future we can just add it back in, but it is useful for using things like 'act' for local testing, that may not have all actions cached locally. |
Yup, trying to review these changes made me particularly aware how much mental bandwidth I was dedicating to, "Oh, that's unnecessary, I can just ignore those 4 lines." Better to not have them there at all. |
This is a complete refactor and update of the release workflow in preparation for signed commits (#1354). - Updates all actions versions - Remove mono containers - Reduce apt installations to only required - Use aws credentials actions instead of unmaintained sync action - Use ghcli for asset uploads instead of unmaintained assets upload action - Breaks apart steps into discrete jobs
This breaks up, cleans up, and aligns the deploy workflow with the rest of the refactoring. Further work could be done to consolidate deploy/release as they are quite similar, but that can be left as a future exercise
Contains function was backwards, issue with needs wildcard couldn't be replicated. Co-authored-by: HebaruSan <HebaruSan@users.noreply.github.com>
Co-authored-by: HebaruSan <HebaruSan@users.noreply.github.com>
2bab242
to
79cf126
Compare
Alright, I think that's everything! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, looks good to me. Let's go!
This is a complete refactor and overhaul of our actions, and either updating or changing any outdated actions we have used within our workflows.
Changes
Building
The build workflow is now a self contained, and runs on master or via 'workflow_call'. Making a single source of truth for building CKAN/NetKAN.
Note: The build on master is intentional, I expect there we can do a small amount of work to enable dotnet caching. For a cache to be available on branches, it must have been previously built on the default branch.
Testing
There is a new test file that calls build first. It is unnecessary to build inside the Mono containers, as we still use dotnet to build. This however ensures we are keeping the same validation as before. We can likely look to considering if this is still required.
Deploy
Deployment now calls build first, followed by versioning, and then the rest of the steps.
Example here: https://github.com/KSP-CKAN/CKAN/actions/runs/8948785797
Example with containers (fails as I omitted auth): https://github.com/KSP-CKAN/CKAN/actions/runs/8948756697/job/24582270638
Release
Release has gone through a similar tidy up to deploy. Arguably a shared workflow could be built for this, but it felt prudent to keep it separate for now. When signing is fully available we can take a look at it then.
Example here: https://github.com/KSP-CKAN/CKAN/actions/runs/8936371852
Other things of note
/test
path for all S3 uploads, which look to have worked in the same manner as the previous action.