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

Cloud Run pipeline to staging #69

Merged
merged 28 commits into from
Jun 21, 2021
Merged

Cloud Run pipeline to staging #69

merged 28 commits into from
Jun 21, 2021

Conversation

dinagraves
Copy link
Contributor

@dinagraves dinagraves commented Jun 11, 2021

This pipeline:

  • Builds the website
  • Deploys to Cloud Run with 0% Traffic
  • Slowly rolls out traffic until 100%

This setup requires creating triggers, artifact registry repos, pub/sub topics, and adjusting permissions. Should I include the terraform in this PR or in a followup?

Other question, should I include a step that prints out the pub/sub message for debugging?

Staging website here: https://website-qo2ri6lf3a-uc.a.run.app

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 11, 2021
ops/canary.cloudbuild.yaml Outdated Show resolved Hide resolved
ops/canary.cloudbuild.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@grayside grayside left a comment

Choose a reason for hiding this comment

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

No blockers at this time, but I've asked a couple clarifying questions that could lead to blockers or follow-ups.

@@ -0,0 +1,26 @@
# Copyright 2021 Google LLC
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should Documentation & Decision records be blockers or follow-up issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unsure - maybe should be discussed at our meeting today?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems we missed this in meeting. My general preference is that each PR includes any relevant decision records, but my position on this is flexible while we figure out how that impacts velocity.

- 'build'
- '-t'
- 'us-central1-docker.pkg.dev/${PROJECT_ID}/${_DIR}/${_DIR}:${SHORT_SHA}'
- '${_DIR}/.'
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is ${_DIR}/. mean to enable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was to be able to use the same builder for the website and content-api. The build initiates from the root directory and dives down into _DIR/ to find the appropriate Dockerfile.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's worth having a comment that this builder is used for all container images. This is also worth a decision record.

I think there's definitely value here in creating a unified build process, especially if this is where we ensure provenance, add binauth attestation later on, etc.

ops/build.cloudbuild.yaml Outdated Show resolved Hide resolved
# TODO: Run tests, revert back to previous revision if failure

# Cloud Run Canary
- id: 'Update Traffic'
Copy link
Collaborator

Choose a reason for hiding this comment

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

comment: I think this is a good starting place, but I expect we'll iterate significantly in defining how canary rollouts work. How does status that a rollout is partial/complete get reported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's writing to a pub/sub topic, so we can subscribe to that. We can also see it in the Cloud Run UI.

- --platform=managed
- --revision-suffix=${_REVISION}
- --project=${_TARGET_PROJECT}
- --allow-unauthenticated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why unauthenticated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking for Prod it would have to be unauthenticated and I want the staging rollout to be as much like the prod one as possible.

Note, this is all after all the checks on the PR and it's merged into main. So at this point there should have already been a private service built and tested against as part of an e2e test. For the CD pipeline, even though this is staging, I wanted it to be as much like prod as possible.

@dinagraves
Copy link
Contributor Author

@grayside and @ace-n Thank you for the reviews! I have a couple outstanding questions in the PR description that I'd love y'all to weigh in on. Specifically:

  • Should I include the terraform in this PR or in a followup?
  • Should I include a step that prints out the pub/sub message for debugging?

@dinagraves dinagraves requested a review from grayside June 14, 2021 22:17
@ace-n
Copy link
Contributor

ace-n commented Jun 14, 2021

RE Should I include a step that prints out the pub/sub message for debugging?

My 2¢: maybe, but comment it out if you do? That way it might function as a "teachable moment" on how to debug with Pub/Sub.

@grayside
Copy link
Collaborator

Should I include the terraform in this PR or in a followup?
I think not, these are two significant pieces. In the spirit of DORA, small changes shipped often.

Should I include a step that prints out the pub/sub message for debugging?
Sounds good, it often carries helpful context.

Adding debug print statement
Copy link
Collaborator

@grayside grayside left a comment

Choose a reason for hiding this comment

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

Approving change, with note that we have multiple decision records to add here or follow-up.

I've raised larger design questions in the separate doc but this is a good starting place for iteration.

@@ -0,0 +1,26 @@
# Copyright 2021 Google LLC
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems we missed this in meeting. My general preference is that each PR includes any relevant decision records, but my position on this is flexible while we figure out how that impacts velocity.

- 'build'
- '-t'
- 'us-central1-docker.pkg.dev/${PROJECT_ID}/${_DIR}/${_DIR}:${SHORT_SHA}'
- '${_DIR}/.'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's worth having a comment that this builder is used for all container images. This is also worth a decision record.

I think there's definitely value here in creating a unified build process, especially if this is where we ensure provenance, add binauth attestation later on, etc.

ops/deploy.cloudbuild.yaml Outdated Show resolved Hide resolved
@dinagraves dinagraves merged commit 110e1e6 into main Jun 21, 2021
grayside pushed a commit that referenced this pull request Jul 22, 2021
Initial Cloud Run CD pipeline
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants