-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: support review apps #799
Conversation
55367a5
to
275b320
Compare
This adds in an option to run hako deploy with an "ephemeral" app, which is a similar concept to Heroku review apps. To make this work we need to pass Hako config into the Docker command as STDIN. This also updates the two wrapper plugins to introduce a `deploy:review` command. This was all tested locally in cp-docker-migration-canary. It works aside from an issue with name length, we're talking to Cloud Enablement about this. Co-Authored-By: Alex Muller <[email protected]>
275b320
to
ad5a0d4
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.
woohoo 🙌🏽 Just 1 suggestion from me!
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.
LGTM, great work. Thanks
throw new Error(`Hako config not found at ${hakoConfigPath}`) | ||
} | ||
} | ||
child.stdin.end() |
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.
praise: beautifully done
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.
👌🏽 Lets go!
// Because we can't mount volumes in Docker images on CircleCI we have to | ||
// pass the hako config via STDIN |
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.
question: why can't we include the hako-config
file in the Docker build context instead of passing it in at runtime?
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.
Because of this, Docker is incapable of mounting volumes (and thus reading files) in CircleCI unless you use a machine executor which would require us to make a lot of CircleCI config changes.
More detail here, sorry I should have linked this.
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.
Oh wait that wasn't what you were asking. Because the docker run
in question is Hako, not the image we built
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.
Sorry, I mean why can't we include the hako-config/apps/.../app.yaml
file in the Docker image we build in the DockerBuild
task – but the answer is because we're running the hako image here not the image we've built (edit: like you say) 😰 This approach makes sense then (though I hope we'll get to a point where Hako doesn't (officially) require Docker some day)
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.
Yeah I'd like that too 😂 would simplify some things but I'm sure introduce some fun different issues. The stdin stuff was also a big pain for Cloud Enablement to implement
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.
🎉
Description
This adds in an option to run hako deploy with an "ephemeral" app, which is a similar concept to Heroku review apps. To make this work we need to pass Hako config into the Docker command as STDIN.
This also updates the two wrapper plugins to introduce a
deploy:review
command.This was all tested locally in cp-docker-migration-canary. It works
aside from an issue with name length, we're talking to Cloud Enablement about thisfixedThis relies on https://github.com/Financial-Times/hako-cli/pull/324
Checklist:
feat(circleci): add support for nightly workflows
,fix: set Heroku app name for staging apps too