-
Notifications
You must be signed in to change notification settings - Fork 18
Conversation
I've changed the logic to always run the deploy script, and to just exit 0 if there CIRCLE_BRANCH doesn't match a I'd be okay with having circle_deploy release to fr-stage on every build, and have merge to master release to production, dashboard.fr.cloud.gov. |
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.
Got some questions. Is this already deploying to staging somewhere? Do you have a CircleCI link I can look at?
@@ -39,7 +39,7 @@ then | |||
CF_APP="cg-dashboard-demo" | |||
else | |||
echo Unknown environment, quitting. >&2 | |||
exit 1 | |||
exit 0 |
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.
Why are we exiting cleanly here? Shouldn't it stay as exit >0
in order to fail?
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 old logic only called the deploy script if it had one of the appropriate branches or a matching tag.
To replicate that same behavior with 2.0, I'd have to use a filter statement, which would either be in the workflow, as a deploy step, or in the jobs. That would double the number of jobs/steps, so each job would be held up by 22.9Gb Docker install, and we'd have to make sure we'd cached and restored all the correct paths between the jobs.
Since the deploy script has the logic to deploy to the correct place, there's no harm in having it called with each job, but just exit 0 (OK) if there's no matching tag/branch.
I know this whole PR is ugly as sin, so I hope we tamp down our aesthetic inclinations and move it forward.
# In CircleCI 2.0 you can now specify your own image, or use one of our pre-configured images. | ||
# The following image, at 22.9 Gb, is the old pre-configured image: | ||
docker: | ||
- image: circleci/build-image:ubuntu-14.04-XXL-upstart-1189-5614f37 |
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.
Is there a reason we can't use a different image here? Like one specific for the tasks that it's accomplishing, go
, node
, etc?
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.
Yes, there is a reason. I tried to go the "correct" route by having the appropriate Docker container for the Go lang stuff, the Node stuff, the tests, etc. But it turned into a rabbit hole, first because the different images have different permissions on their paths, so I could not cache the node modules from one image and then have it restored on a subsequent Go image, because the gopath is rooted at /go/src
Second, the dependencies didn't align exactly with the way we'd been doing things before, and I was starting to get failed test results. As I started to backtrack those, I realized I was consuming a lot of my time, and that I would probably optimize for human time by just using the kitchen sink image and doing things pretty much the way they'd be done before.
You can see a lot of my abortive work trying to go the right direction in https://github.com/18F/cg-dashboard/compare/pdb-ci2.2 -- then as I got stuck trying to resolve issues with fibers binaries and node-gyp, I decided to chuck it all and just build everything off of the fat image.
working_directory: ~/18F/cg-dashboard | ||
command: sudo tar -C /usr/local -xzf download/$GODIST | ||
# Dependencies: | ||
- run: sudo apt-get update; sudo apt-get install libicu52 |
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.
What are these doing?
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.
Don't know. It was in the old circle.yml
Maybe it's not needed, but I don't see the harm in leaving it here.
Answered all questions. No changes needed.
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.
but not merging. Leaving that to @pburkholder
Don't merge until changing #L89 of config.yml to not force the
CIRCLE_BRANCH setting