Add workaround for CircleCI's problems filtering from forks #4505
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## develop #4505 +/- ##
===========================================
+ Coverage 82.62% 82.63% +<.01%
===========================================
Files 45 45
Lines 3114 3115 +1
Branches 337 337
===========================================
+ Hits 2573 2574 +1
Misses 456 456
Partials 85 85
Continue to review full report at Codecov.
|
redshiftzero
reviewed
Jun 6, 2019
CircleCI's branch filtering does not work properly with pull requests from forks. The CIRCLE_BRANCH variable contains something like freedomofpress/pull/3 in this case. This means that docs and i18n PRs from forks are not being tested as we wish; the translation tests are not run for i18n PRs, and the app tests are being run for docs PRs. A CircleCI feature request [1] to improve this was closed without explanation, so I'm incorporating a workaround suggested in the CircleCI forums: using the GitHub API to obtain the real branch name for PRs from forks, and skipping tests if it doesn't match. Not all steps of the relevant jobs are skipped, but the most expensive ones are. Also, stop skipping static-analysis-and-no-known-cves, as it doesn't take that long, and might prevent problems from sneaking in on branches with inaccurate names. [1] https://discuss.circleci.com/t/only-build-pull-requests-targetting-specific-branch/6082/6
redshiftzero
suggested changes
Jun 8, 2019
.circleci/config.yml
Outdated
| @@ -292,7 +320,13 @@ jobs: | |||
| - run: apt-get install -y make virtualenv python-pip enchant | |||
There was a problem hiding this comment.
we'll need to install jq here too no?
.circleci/config.yml
Outdated
| command: | | ||
| BRANCH_MATCH=$(devops/scripts/match-ci-branch.sh "^update-builder") | ||
| echo "match-ci-branch.sh said: ${BRANCH_MATCH}" | ||
| if [[ $BRANCH_MATCH =~ ^found ]]; then echo "Skipping: ${BRANCH_MATCH}"; exit 0; fi |
There was a problem hiding this comment.
if [[ $BRANCH_MATCH =~ ^found ]] -> if ! [[ $BRANCH_MATCH =~ ^found ]] here since we only want to skip the test when we don't find update-builder in the branch name
(btw: the deb-tests job isn't getting kicked off when testing from a fork since we're using circle ci's branch filtering on /update-builder-.*/. I think this makes sense since in practice anyone who is doing builder updates has write access to the repo.)
Closed
redshiftzero
approved these changes
Jun 10, 2019
emkll
added a commit
that referenced
this pull request
Jun 18, 2019
Changes in develop were introduced in #4505, minimal fix to ensure the dependency is installed and CI passes only in the 0.13.1 release branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Status
Ready for review
Description of Changes
CircleCI's branch filtering does not work properly with pull requests
from forks. The
CIRCLE_BRANCHvariable contains something like/pull/3in this case. This means that docs and i18n PRs from forks are not
being tested as we wish; the translation tests are not run for i18n
PRs, and the app tests are being run for docs PRs.
A CircleCI feature request to improve this was closed without
explanation, so I'm incorporating a workaround suggested in the
CircleCI forums: using the GitHub API to obtain the real branch name
for PRs from forks, and skipping tests if it doesn't match. Not all
steps of the relevant jobs are skipped, but the most expensive ones
are.
Also, stop skipping static-analysis-and-no-known-cves, as it doesn't
take that long, and might prevent problems from sneaking in on
branches with inaccurate names.
Fixes #4479.
Testing
We're going to test this with two PRs, one for docs and one for i18n, to verify that the right tests run and pass.
Deployment
This will only affect CI.
Checklist
If you made non-trivial code changes: