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

Feature Branch Deploys #98

Draft
wants to merge 38 commits into
base: master
Choose a base branch
from

Conversation

joyliu-q
Copy link
Contributor

@joyliu-q joyliu-q commented Oct 24, 2021

Overview

On PRs, deploy a version of the product to something like pr-xyz.domain.com using a sqlite db. This is important for testing site behavior prior to merging to main.

This PR details the infrastructure level changes to enable feature branch deploys. After this PR is merged, additional changes may be necessary to configure feature branch deploys for each product.

Desired Behavior

Feature branch deploys are separate from production deploys in the following:

  • Their domains will be prefixed by the PR number.
  • Inside each container created by the feature branch deploy (application & cronjobs), the IS_FEATURE_BRANCH env variable will be set for any additional configurations on the application-side.
  • The database will likely be sqlite instead of postgres and populated with dummy data, but this depends on the product-specific set-up.

When are feature branch deploys created? They are created when you open a PR AND the name of that PR branch starts with feature (e.g. feat/add-search-bar). These feature branch deploys should be updated whenever there is a change made within that PR, and then nuked when the PR is closed.

Usage

To use feature branch deploys, set enableFeatureBranchDeploy = true in your LabsApplicationStack inside kraken. This will add additional configuration to your build-and-deploy workflow and create an additional workflow file that nuke feature branch deploys when PRs closes.

Infra

Kraken changes

TLDR: For the LabsApplicationStack, add a custom field enabling feature branch deploy workflows to be generated.
This PR also introduces a NukeJob, which... nukes stuff >:))

On PR Open

Generate a new set of deployment config with the staging set-up. *Note: might need to adjust because this is not just on PR open but also whenever the feature PR is being updated

  • Set IS_FEATURE_BRANCH to true
  • Set PR_NUMBER to ${{ github.event.number }}
  • Set RELEASE_NAME to penn-clubs-pr-xx

On PR Close or Merge.

Kill everything specified in the deployment config (make sure to test this)

TODOs

  • Maintain Default Config for production deployments
    • Test on a product when upgrading kraken, the default workflows remain the same
  • On PR Open (non-master): deploy feature branch
    • Make it
    • Test it
  • On PR Close or Merge: nuke feature branch deployment
    • Make it
    • Test it

MAKESUREs

  • Deployment Configuration
    • Update the RELEASE_NAME env variable
    • Use subdomain
    • The DJANGO_SETTINGS_MODULE is set up properly such that it reads the staging file because by default we specify it as production in our images (thought about this and should work in theory)

Testing & Clean-up

  • Update snapshot tests
  • Add new snapshot tests
  • Verify that new workflows work as intended
  • Bump version & update changelog

Backend

Add env variable check to populate database for feature branch deploys

This change is necessary to accommodate for feature branch deploys.

Currently, we run the migrations such that the database exists, but for feature branch deploys we want to fill it with dummy data for testing purposes. This can be done by checking for a RUN_POPULATE env variable in the django base & follow-up changes in using the django base.

Misc

Penn Courses now uses postgresql even in development, so will need to accommodate for that.
There are also other things that the backend of products use (v1 ignoring specific services? S3 bucket makes sense, but things like twilio doesn't really belong in feature branch deploys/staging anyways)

These are NOT handled by this PR, so additional configuration would be necessary.

Frontend

Frontend Changes

On the application side (NOT handled by this PR):

  • Make sure DOMAIN is updated.
  • Ensure that the frontend is using a publicRuntimeConfig to ensure that the DOMAIN environment variable is actually respected during runtime and not hardcoded at build time.

Adoption:

  • Add branch protection rule against feat/ so randoms can't open a feature branch and deploy vulnerable code.

@joyliu-q joyliu-q force-pushed the feature/add-run-populate-for-feature-branch-deploys branch from 9773dbc to dbb89d2 Compare October 24, 2021 17:15
@@ -1,7 +1,13 @@
#!/bin/bash
set -e
Copy link
Contributor Author

Choose a reason for hiding this comment

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

set -e makes it immediately exit if there's a non-zero status

@joyliu-q joyliu-q changed the title Add env variable check to populate database for feature branch deploys Feature Branch Deploys Apr 12, 2022
@codecov
Copy link

codecov bot commented Apr 12, 2022

Codecov Report

Merging #98 (c887d91) into master (272b897) will increase coverage by 2.64%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master       #98      +/-   ##
===========================================
+ Coverage   97.35%   100.00%   +2.64%     
===========================================
  Files          16        16              
  Lines         265       218      -47     
  Branches       73        39      -34     
===========================================
- Hits          258       218      -40     
+ Misses          5         0       -5     
+ Partials        2         0       -2     
Flag Coverage Δ
kittyhawk ?
kraken 100.00% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cdk/kraken/src/cdk.ts 100.00% <100.00%> (ø)
cdk/kraken/src/deploy.ts 100.00% <100.00%> (ø)
cdk/kraken/src/index.ts 100.00% <100.00%> (ø)
cdk/kraken/src/labs-application.ts 100.00% <100.00%> (ø)
cdk/kraken/src/nuke.ts 100.00% <100.00%> (ø)
cdk/kraken/src/postintegrationimagepublishjob.ts 100.00% <100.00%> (ø)
cdk/kraken/src/utils.ts 100.00% <100.00%> (ø)
cdk/kittyhawk/src/application/base.ts
cdk/kittyhawk/src/cronjob.ts
cdk/kittyhawk/test/utils.ts
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 713bdf8...c887d91. Read the comment docs.

@joyliu-q joyliu-q marked this pull request as draft April 12, 2022 23:07
@ArmaanT
Copy link
Member

ArmaanT commented Apr 13, 2022

A couple of comments:

  1. For frontend changes, you'll also have to ensure that the frontend is using a publicRuntimeConfig to ensure that the DOMAIN environment variable is actually respected during runtime and not hardcoded at build time.
  2. What's your plan for handling databases and other external services each product needs (ex. s3 buckets, twilio, shibboleth, etc)?

cdk/kraken/src/deploy.ts Outdated Show resolved Hide resolved
@joyliu-q joyliu-q changed the title Feature Branch Deploys Feature Branch Deploys (Kraken Changes) Apr 19, 2022
@joyliu-q joyliu-q changed the title Feature Branch Deploys (Kraken Changes) Feature Branch Deploys Apr 19, 2022
@joyliu-q
Copy link
Contributor Author

joyliu-q commented Apr 21, 2022

We will need to adjust one of the workflows because we don't just want to deploy when the PR is created, but rather update the deployment configuration whenever the PR is updated.

Switching from pr-open-based to push-based, we should limit feature branch deploys to branches prefixed with feat/xyz, and set it up such that it does feature branch deploy whenever changes are pushed to a branch prefixed with feat. At the same time, we still want to know which PR number it is. This method looks promising, so will be trying it out.

The nuke job may not need to be adjusted, as the yaml generated from it will likely be the same as the last feature branch deployment from the last commit on that PR.

cdk/kraken/src/deploy.ts Outdated Show resolved Hide resolved
@joyliu-q
Copy link
Contributor Author

Also, haven't figured out how to comment the feature branch deploy link(s) because sometimes there ARE no accessible links, and sometimes there are multiple. This is more product specific, so oh well. Will figure it out.

Worst case scenario, you just see how the github actions succeeded and you manually go to the link yourself (<- This is kind of like a staging vibes so honestly might as well make a staging branch for every product to merge into instead of all this feature prefix stuff). But then again it's nice to have multiple going at the same time and it's less permanent.

@joyliu-q
Copy link
Contributor Author

joyliu-q commented Apr 21, 2022

TODO: need to handle building & publishing the docker images. Still haven't figured out the desired behavior for that: should it make a new docker image suffixed with the PR number, and then delete that image when the PR closes? That seems like a reasonable way but making temporary docker images is not very clean.

Alternatively, something like staging could handle build & published docker images, and we can just use the original one for feature branch deploys :D

@@ -73,6 +73,11 @@ export class DeployJob extends CheckoutJob {
{
id: "synth",
name: "Synth cdk8s manifests",
...(fullConfig.isFeatureDeploy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider moving the check for PR number earlier to skip feature branch deploy job as a whole


# Get repo name (by removing owner/organization)
export DEPLOY_TO_FEATURE_BRANCH=true
export RELEASE_NAME=${"${REPOSITORY#*/}-pr-${{ steps.pr.outputs.pull_request_number }}"}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You may be wondering, "Joy, why are you doing this dumb thing RELEASE_NAME=${"${REPOSITORY#*/}}? Couldn't you just escape it using RELEASE_NAME=\${REPOSITORY#*/}

The answer is that ts-dedent is clowning me. I spent too much time trying to figure out what is going on. This is a bug, so I'm using this very sad format to do it. Hopefully someday they fix it and I'll use the clean way.

@joyliu-q
Copy link
Contributor Author

joyliu-q commented Jun 28, 2022

Design Consideration: Announcing Feature Branch Deploys

Right now, we deploy feature branches to pr-#.pennlabs.org, but to an average Jo, it can be hard to infer that information.

Method

Solution 1. Send "Successful Deployment" message for each commit pushed (Current)

Pros:

  • Spammy, but could encourage people to think through their code before pushing. (Somewhat doubtful).
  • Easy to do. No extra workflow files.
    Cons:
  • Too spammy. Every time someone comments on a PR, you get an email notification. I can easy see someone pushing 50 commits into a feature branch and getting bombarded with 50 emails from Github.

Solution 2. Send 1 singular message at the first time the PR is opened.

Pros:

  • Not spammy.
    Cons:
  • Message could potentially get post among sea of info.
  • Extra workflow file.

Solution 3. Change the PR descriptions once to include the link.

Pros:

  • Link will never be lost among the sea of comments.
    Cons:
  • Could be a random/strange developer experience to suddenly see a random URL pop up on the PR description you just wrote.
  • We don't want this to just happen once when the PR is open, because what if at the first commit the CI fails & deployment never happened?

What does Vercel do?

Vercel offers something similar for Github Actions. With Vercel, you give it the alias-domains (similar to our deploymentUrls), and it makes a single comment on your PR.

End solution

Solution 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants