-
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
Run staging CI in integration branch #2538
Comments
IMHO there is no immediate need for this (fine) adjustment. The problem happened when a contributor tried to improve the CI. They are bound to do mistakes that use more resources than needed. And if they can't (because they have to first wait for their change to be merged in the integration branch). it may discourage them from contributing. Another concern is that adding an integration stage adds complexity to the contribution process (not much but still). That being said, I think it would make sense to implement what is proposed when and if some malicious person sends a stream of pull requests designed to waste resources. I don't feel strongly about this but I tend to favor not doing anything ;-) My 2cts. |
A closely related idea that would be better is running staging tests on demand (e.g. triggered by a comment in a PR) as it:
|
@msheiny just pointed me to https://circleci.com/docs/2.0/workflows/#holding-a-workflow-for-a-manual-approval which would address the concern here: we can prevent the staging job from running on forks and just hold the staging CI job until we've looked at the diff, think it's ready for a staging test run. The workflow is that we would need to go in Circle CI and approve the job run. This way we can sidestep having to workaround the CI restrictions here, e.g. needing to separately trigger release candidate builds in another repo (freedomofpress/securedrop-builder#63). |
Closing; we're likely to move some jobs to nightly (e.g., #5336), but are unlikely to implement the strategy described here. |
Feature request
Description
In a recent hack-a-thon an issue came up where CircleCI was not properly building on forks of users that already had existing CircleCI accounts. In particular, was not importing all of our environment variables and instead using the fork author's circleCI environment. See comments in #2515.
That PR also ^^^ added an additional x2 builders for a total of x4 builders per run - which I see as problematic because of the increased complexity of the CI job and potential for annoying to debug issues to get CI to successfully pass (which has happened in the past a ton).
I foresee these two issues combining to cause significant more issues for new contributors and at hackathons (where we'll have mutliple folks trying to contribute at the same time rapidly).
I'd like to see the CI workflow refactored as follows:
application-tests
(if we want to wait to refactor that to containers, thats fine with me .. the current travis job is clunky). If passes, maintainers will merge that into anintegration
branch.integration
with the full suite of tests and if failures are detected we notify ALL chat channels that CI needs to get fixed. Otherwise, if everything passes, CI will automatically merge integration --> develop.There are definitely cons here -- but ultimately very few contributors are touching the ansible logic and yet we are spinning up a lot of flaky environments that have potential to fall over and cause grief for every single PR. This will require the team to be more aware of the pipeline and respond when CI starts failing in develop.
However as pros I see:
The big blocker for this effort that I see is that the app-tests really need to get containerized properly first. @tmc brought in PR (#2523) which aims to address this. If that goes in, I'm not sure it'll answer all pre-requisites but its certainly a great start.
User Stories
As a contributor, I want CI to be very stable, run quickly, and get potential accepted code merged efficiently by the SecureDrop maintainers.
As a maintainer, I want the CI pipeline to run in discrete chunks that is easier to respond to when integration issues arise. Having one CI job to maintain versus one for each PR to analyze.
The text was updated successfully, but these errors were encountered: