-
Notifications
You must be signed in to change notification settings - Fork 31
Merge all native platform specific workflows into 1 again #625
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
Merge all native platform specific workflows into 1 again #625
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #625 +/- ##
=======================================
Coverage 77.74% 77.74%
=======================================
Files 9 9
Lines 3757 3757
=======================================
Hits 2921 2921
Misses 836 836 🚀 New features to boost your workflow:
|
c908f46
to
24dde1e
Compare
@@ -0,0 +1,354 @@ | |||
name: Native Builds |
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.
@vgvassilev The name is the only thing I wasn't sure about. Main/CI didn't seem useful as a name. For now I just put native builds, since that is what the jobs are. I could shorten it to native, since the other workflow just has Emscripten. What do you think?
At some point I plan to combine this with the Emscripten workflows. At which point it could just be called Builds.
02d5606
to
c6673b3
Compare
@mcbarton is this PR ready for review? |
@aaronj0 Yes, this is ready for review. If you approve of the PR, then I will merge it. I want to rebase and merge this PR, rather than squash and merge like I usually do, as the commits are meaningful, since they are about the creation and use of individual reusable actions. |
I believe the first 5 commits should land as one commit since we can have a single point where the workflows were made reusable. If you can do that then yes feel free to merge as 2 commits. |
I should make clear that the workflows and actions are still not reusable across repos at this point. I know I could not use them in cppyy related repos, which is the end goal. They still need modifying to be reusable, which will be done in a subsequent PR. I'd still like to keep these 5 commits separate. |
I'd prefer having atomic and single commits for the CI. We probably are dominated by ci changes in the development which is probably not the best sign we send to the outside observers. |
I agree with this, and now that I look at it, the documentation update can also go into the same commit, since it's a readme update based on the single workflow file added in the previous commit. Those are not atomic... can you squash and merge? |
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.
LGTM, given this is squashed for merging
If you want to squash and merge, then go ahead, but I want to make it clear that I am quite unhappy with this. I made an effort to make a series of reasonable commits, each with a clear objective to getting to this stage of being able to have one workflow file again. To me merging them into a single commit, makes it less clear how this was achieved, and disregards the effort needed to make this happen. |
This is one of those cases where the commits don't reflect discrete functional changes to our CI infrastructure, that make sense to have in the history. That being said, squashing won’t erase your effort or make it any less visible; Thanks again for all the great work in getting us back down to a single workflow file with reusable actions. |
Description
Please include a summary of changes, motivation and context for this PR.
Once this PR is finished, all the native platform specific workflows should be merged into 1 workflow file. e.g. main.yml
Fixes # (issue)
Type of change
Please tick all options which are relevant.
Testing
Please describe the test(s) that you added and ran to verify your changes.
Checklist