-
Notifications
You must be signed in to change notification settings - Fork 696
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
Ensure CI staging run reports errors accurately #4089
Ensure CI staging run reports errors accurately #4089
Conversation
The `make ci-go` target was always exiting zero, due to the in-line bash if statement, causing failed CI runs to report as successful. Moved the logic into a wrapper script. Also trimmed out from of the extraneous Makefile targets related to CI (and corresponding docs), to remove a bit of the indirection and make maintenance more straightforward for the team.
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.
The current changes look okay. @conorsch I am guessing next you will add a failed commit.
ae6b630
to
fb13e86
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.
Visual diff and functional tests look good to me @conorsch. I've taken the liberty of slightly amending the test plan (not reverting the commit / waiting for yet another CI run), see comment above. Happy to do so in a 2nd review pass if you think it's crucial.
👍 to merge once CI passes again.
LGTM! Thanks for review and final testing bits, @emkll. Let's watch CI runs in the future—we should see a slight uptick in failures, but those failures should be legitimate. If any flakiness occurs due to e.g. connection timeouts, we'll track that separately. |
Status
Work in progress
Description of Changes
Fixes #4066
The
make ci-go
target was always exiting zero, due to the in-line bashif statement, causing failed CI runs to report as successful. Moved the
logic into a wrapper script. Also trimmed out from of the extraneous
Makefile targets related to CI (and corresponding docs), to remove a bit
of the indirection and make maintenance more straightforward for the
team.
Testing
We'll have to make temporary commits on the WIP PR to confirm that an error in e.g. the provisioning flow results in failed CI. Rough checklist is below. All reports should include URLs to the corresponding CI run satisfying the requested check.
fail
task); confirm observation of failure in CIIt's possibly we've been missing CI-specific problems such as connection timeouts, so we may need to add retries or 😱 sleeps in order to stabilize.
Deployment
None, CI only.
Checklist
If you made changes to the server application code:
make ci-lint
) and tests (make -C securedrop test
) pass in the development containerIf you made changes to
securedrop-admin
:make -C admin test
) pass in the admin development containerIf you made changes to the system configuration:
If you made non-trivial code changes:
If you made changes to documentation:
make docs-lint
) passed locally