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

Add granularity to ci pipeline for readability #1546

Merged
merged 4 commits into from
Aug 24, 2022

Conversation

gonzalo-bulnes
Copy link
Contributor

@gonzalo-bulnes gonzalo-bulnes commented Aug 17, 2022

Description

🔮 Reviewers: This PR is ready to review, but includes three commits from #1544 because they make the work easier. Only the very last commit (0a6d4d7 click me!) needs to be reviewed here. (I'll update this branch as soon as #1544 is merged.)

Splits the testing, linting, and diverse checks into distinct CI jobs.
Bonus: a ~20% decrease in wall clock time (the trade-offs are called out in #1543).

Fixes #1543

Result

Preview of the distinct CI jobs, with a duration of 3:40min

Test Plan

  • Confirm that all the checks contained in make check are performed at some stage of the pipeline
  • Confirm that the grouping of checks makes sense
  • Confirm that CI is green 🍏

As far as I can tell, the Circle CI convenience scripts
(e.g. the script that uploads test results) should still work,
because they work with debian:buster.
Minor refactor: DRY step definitions where possible
@gonzalo-bulnes gonzalo-bulnes requested a review from a team as a code owner August 17, 2022 04:06
@gonzalo-bulnes gonzalo-bulnes added 🏖️ Summer cleanup 🍓 Bite-sized This pull request should be very quick to review. labels Aug 17, 2022
@gonzalo-bulnes
Copy link
Contributor Author

Note: the repo configuration is a blocker, since there is no way the removed (Buster) jobs will report their status to GitHub 🌵
I believe it should be possible to ask GitHub to make all checks required? And I suppose that would automatically adapt to changing check lists?

@gonzalo-bulnes gonzalo-bulnes added the ⚙️ Tooling Improving maintainability and increasing maintainer joy : ) label Aug 17, 2022
@eloquence
Copy link
Member

No strong feelings from me on this change; it is worth noting I think that if we add Bookworm to the mix, that's a lot of individual jobs to keep track of. My own preference might be for a bit more grouping esp. of similar very short-running jobs.

@gonzalo-bulnes
Copy link
Contributor Author

To clarify my intent: my experience is that on the day-to-day we only ever have to navigate Circle CI's list of jobs when one is failing. At that point, I'm trying to make finding the failure cause quicker/easier by reducing the job log size, and the search pattern consistent.

If a unit-tests fail, then you'll be able to confidently scroll to the bottom of the log. Same for any other job. Currently, the one job fails and you need to scroll up and down a way longer log, until you find where the error was printed. (You can also not effectively search for "error", nor "FAILURE", nor most things you may think would help you.)

@gonzalo-bulnes
Copy link
Contributor Author

Can you clarify what you mean by "keeping track of jobs" @eloquence ?

@eloquence
Copy link
Member

I was just referring to the number of checks that appear at the end of each PR that reviewers have to pay attention to. It looks like GitHub will always group failing checks at the top, so I think in practice it may not be much of an issue if there are 4 checks or 18 (once we add Bookworm). In the securedrop-debian-packaging repo we already have 20 checks and it doesn't seem to be causing any practical inconveniences.

If you find the higher granularity helpful, and if others have no concerns with this approach, I'd be happy to merge this tomorrow and update the repo settings to mark all checks as required.

@rocodes
Copy link
Contributor

rocodes commented Aug 24, 2022

Thanks for preparing this, @gonzalo-bulnes - I'm in favour of this change. Do you want to do a rebase/amendment of history now that the commits from #1544 are in?

Copy link
Member

@eloquence eloquence left a comment

Choose a reason for hiding this comment

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

LGTM, let's give it a try! (There might be further refactoring opportunities to reduce some duplication.) (Edit: Sorry, missed your rebase/amend note Ro!)

@eloquence eloquence merged commit 7099d9c into main Aug 24, 2022
@eloquence eloquence deleted the add-granularity-to-ci-pipeline-for-readability branch August 24, 2022 16:18
@cfm
Copy link
Member

cfm commented Aug 30, 2022

Thanks for proposing this change, @gonzalo-bulnes! It's a gift.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏖️ Summer cleanup ⚙️ Tooling Improving maintainability and increasing maintainer joy : ) 🍓 Bite-sized This pull request should be very quick to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Identifying test failures in the CI pipeline is attention-consuming
4 participants