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

chore(ci): skip build & deploy of content-server docker image for PRs #2620

Merged
merged 1 commit into from
Sep 27, 2019
Merged

chore(ci): skip build & deploy of content-server docker image for PRs #2620

merged 1 commit into from
Sep 27, 2019

Conversation

lmorchard
Copy link
Contributor

@lmorchard lmorchard commented Sep 25, 2019

TL;DR: This can possibly shave 10 whole minutes off CI times.

  • split build & deploy out of test-content-server
  • should still build & deploy for master branch and tagged releases

fixes #2526

@lmorchard
Copy link
Contributor Author

Starting this as a draft to see how CircleCI likes it. Main thing here is to try saving the 6-8 minutes of building a Docker image for content-server that never gets used for tests or pushed anywhere.

@lmorchard
Copy link
Contributor Author

Seems to work as expected on CircleCI - test-content-server is run, but build-and-deploy-content-server is not. However, I'm not entirely sure how to check its behavior for master branch and release tags without merging it. Maybe I can try hooking my personal fork up to CircleCI and merge to master there.

@lmorchard
Copy link
Contributor Author

lmorchard commented Sep 26, 2019

Turned on CircleCI for my personal fork of FxA and merged to master there. Good news is that the build-and-deploy job ran there. Bad news is that the deploy part did nothing, because my CircleCI setup doesn't have the credentials to actually try a push to Docker Hub. But I think that should work in the main repo.

Better news is that the build step took 10 minutes - which is roughly the sort of time that this PR should shave off CI runs for PRs in general.

Screenshot_2019-09-26 Continuous Integration and Deployment(1)

@vladikoff
Copy link
Contributor

@lmorchard please make sure that we can still deploy to docker using feature. and dockerpush. branches:

if [[ "${CIRCLE_BRANCH}" == feature* ]] || [[ "${CIRCLE_BRANCH}" == dockerpush* ]]; then

- split build & deploy out of test-content-server
- should still build & deploy for master branch and tagged releases

fixes #2526
@lmorchard
Copy link
Contributor Author

Okay, pushed a tweak to include /feature.*/ and /dockerpush.*/ branches in triggering the build-and-deploy-content-server job.

No deploy job for 1234-foobar:
Screenshot_2019-09-26 CircleCI - CircleCI(2)

Yes deploy job for feature.foobar:
Screenshot_2019-09-26 CircleCI - CircleCI(1)

Yes deploy job for dockerpush.foobar:
Screenshot_2019-09-26 CircleCI - CircleCI

@lmorchard
Copy link
Contributor Author

lmorchard commented Sep 26, 2019

Hmm, actually - I hadn't heard of this feature.* / dockerpush.* convention for branch names before now. We should probably document that somewhere.

But also, maybe I can push to one of those kinds of branches on the mozilla/fxa repo to test things out before actually going for a merge to master.

Edit: I tried it - hope I didn't blow anything up - but it seems to work with a dockerpush.* branch on mozilla/fxa:

Screenshot_2019-09-26 CircleCI

Screenshot_2019-09-26 Image Layer Details - mozilla fxa-content-server dockerpush testpleaseignore - sha256 0b32406b37d81fd

@lmorchard lmorchard marked this pull request as ready for review September 26, 2019 23:51
@lmorchard lmorchard requested a review from a team September 26, 2019 23:52
@lmorchard
Copy link
Contributor Author

Going to open this up for review. Seems like it's working, but could use some more eyes to surface anything else I might break like pushing to special branches.

Copy link
Contributor

@chenba chenba left a comment

Choose a reason for hiding this comment

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

👍

@lmorchard
Copy link
Contributor Author

Going to merge... if this breaks anything, it should be pretty easy to revert 😅

@lmorchard lmorchard merged commit a1a0036 into mozilla:master Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skip Docker build step at end of test-content-server
4 participants