Migrate CircleCI workflows to GitHub Actions (3/3) #3368

AzfaarQureshi · 2020-10-19T23:57:58Z

What this PR does

This is PR 3/3 of migrating Cortex's CI from CircleCI to GitHub Actions:

Part 1/3

create test-build-deploy workflow under .github/workflows
add readme
adding the 3 base jobs
- lint
- test
- build
these jobs are run async and share no dependencies with each other
workflow is run when a commit is pushed to master or when any PR is opened. However CD jobs will only run on the master branch and will be skipped otherwise
all jobs in following PRs are dependant on one or more of these jobs finishing

Part 2/3

adding jobs dependant on build
- integration
- integration-config-db
This PR completes the CI portion of migration. The following PR will finish the remaining CD portion of the migration

Part 3/3 👈

This PR completes the migration from CircleCI to GitHub Actions
- deploy_website is dependant on build, test
- deploy is dependant on build, test, lint, integration, integration-config-db
CD jobs will only run on the master branch and will be otherwise skipped

Which issue(s) this PR fixes:

fixes #3274

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pracucci · 2020-10-20T07:06:02Z

@AzfaarQureshi I've merged the part 2 PR. Could you rebase master, please?

pracucci

Thanks for working on it! I left few minor comments, but overall LGTM (good job!). Please remember to rebase master.

As soon as we merge this, the deploy and deploy_website jobs will both run in CircleCI and GitHub actions, effectively deploying the same thing twice. I would suggest to comment out (not remove for now) the deploy and deploy_website trigger from CircleCI so that we'll only run the GitHub actions once (and we'll soon realise if there's any issue, and in case we'll reiterate on it to fix it).

pracucci · 2020-10-20T07:18:29Z

.github/workflows/test-build-deploy.yml

+      - name: Checkout Repo
+        uses: actions/checkout@v2
+        with:
+          ssh-key: ${{ secrets.SSH_PRIVATE_KEY }}


Why is this required, contrary to other jobs?

I'll add a comment for further clarity but to answer your question:

The web-deploy script assumes the repo to be cloned with .git rather than https or else lines like this will return wrong urls. So the ssh key here is required so that web-deploy can correctly interact with github 😄

pracucci · 2020-10-20T07:32:08Z

.github/workflows/test-build-deploy.yml

+          SSH_AUTH_SOCK: /tmp/ssh_agent.sock
+          GIT_SSH_COMMAND: "ssh -o StrictHostKeyChecking=no"


I would add a code comment to explain that the web-deploy script use git to checkout and push to the github pages repository and we use ssh to authenticate to github.

Comment added

pracucci · 2020-10-20T07:33:24Z

.github/workflows/test-build-deploy.yml

+        with:
+          name: website public
+          path: website/public
+      - name: Setup SSH Keys and known_hosts


Suggested change

- name: Setup SSH Keys and known_hosts

- name: Setup SSH Keys and known_hosts used to authenticated to GitHub to deploy website

pracucci · 2020-10-20T07:33:56Z

.github/workflows/test-build-deploy.yml

+          mkdir -p ~/.ssh
+          ssh-keyscan github.com >> ~/.ssh/known_hosts
+          ssh-agent -a $SSH_AUTH_SOCK > /dev/null
+          ssh-add - <<< "${{ secrets.SSH_PRIVATE_KEY }}"


May be worth renaming SSH_PRIVATE_KEY to WEBSITE_DEPLOY_SSH_PRIVATE_KEY for clarity?

pracucci · 2020-10-20T07:36:32Z

.github/workflows/test-build-deploy.yml

+          QUAY_PASSWORD: ${{secrets.QUAY_PASSWORD}}
+          QUAY_USER: ${{secrets.QUAY_USER}}


Similarly to DOCKER_REGISTRY_* I would call these env variables QUAY_REGISTRY_PASSWORD and QUAY_REGISTRY_USER for clarity.

I know you used the same env variable names from CircleCI pipeline, but since they're just used here we can afford to change them for a better consistency without introducing any breaking change on CircleCI.

Standardized registry env variable naming

pracucci · 2020-10-20T07:59:46Z

Assuming you will pick my suggestions, I've already setup the following secrets:

DOCKER_REGISTRY_PASSWORD
DOCKER_REGISTRY_USER
QUAY_REGISTRY_PASSWORD
QUAY_REGISTRY_USER
WEBSITE_DEPLOY_SSH_PRIVATE_KEY

I've intentionally not defined NOQUAY and IMAGE_PREFIX because the defaults should be fine.

shovnik · 2020-10-20T15:15:47Z

Rebased and made all suggested changes @pracucci . I commented out the deploy and deploy-website workflows in CircleCI to avoid deploying twice since they were using the default triggers with restrictions. Let us know if there is anything else we need to change. Thanks for all three reviews!

pracucci

LGTM, thanks! Pinging @pstibrany for the 2nd (required) review and then we can merge. In case of any issue, we can easily disable GitHub actions and de-comment CircleCI.

shovnik · 2020-10-21T18:27:45Z

Sounds good, thanks for the update.

pstibrany

LGTM, thank you for all this work! (my comments/questions are not blocking)

pstibrany · 2020-10-21T18:48:49Z

.github/workflows/test-build-deploy.yml

+      - name: Checkout Repo
+        uses: actions/checkout@v2
+        with:
+          # web-deploy script requires repo to be cloned with ssh for some commands to work


Just curious, which commands require ssh?

I don't see anything ssh-specific in web-deploy script, and I think that using HTTPS with token for pushing should work, but I admit that I only gave it a quick look as this comment piqued my interest.

The current CircleCI workflow also adds a ssh key because git push expects to communicate over ssh (because of how remote is set here. You are right, we could use HTTPS with a token but that would require modifying web-deploy.sh to set the remote url with https://cortexproject<TOKEN>@github.com/cortexproject/cortex.git which would break CircleCI the way it is currently.

As I type this out I realize require might be a strong word because we could do it over https. Maybe rephrasing the comment as "web-deploy script expects repo to be cloned with" would be better?

updated comment!

pstibrany · 2020-10-21T18:50:55Z

.github/workflows/test-build-deploy.yml

+      - name: Setup SSH Keys and known_hosts for Github Authentication to Deploy Website
+        run: |
+          mkdir -p ~/.ssh
+          ssh-keyscan github.com >> ~/.ssh/known_hosts


Do we need to do this, since StrictHostKeyChecking=no is used below?

Yeah I thought so too but removing this section causes a could not resolve host: GitHub.com 😞

pstibrany · 2020-10-21T19:01:53Z

.github/workflows/test-build-deploy.yml

+            docker login -u "$DOCKER_REGISTRY_USER" -p "$DOCKER_REGISTRY_PASSWORD"
+          fi
+          if [ -n "$QUAY_PASSWORD" ]; then
+            docker login -u "$QUAY_REGISTRY_USER" -p "$QUAY_REGISTRY_PASSWORD" quay.io;


Looks like extra semicolon at the end.

I included it here because it's in the circleci config as well: https://github.com/cortexproject/cortex/blob/master/.circleci/config.yml#L247

Do you want me to remove the semicolon in GHA?

Let's keep it for consistency for now.

… changes Signed-off-by: Shovnik Bhattacharya <shovnik@amazon.com> adding comment explaining ssh in web_deploy job Signed-off-by: Azfaar Qureshi <azfaarq@amazon.com>

pull-request-size bot added the size/L label Oct 19, 2020

AzfaarQureshi changed the title ~~Gha migration 3~~ Migrate CircleCI workflows to GitHub Actions (3/3) Oct 19, 2020

pracucci approved these changes Oct 20, 2020

View reviewed changes

shovnik force-pushed the gha-migration-3 branch from f9f2597 to c05fff2 Compare October 20, 2020 14:49

pull-request-size bot added size/M and removed size/L labels Oct 20, 2020

shovnik force-pushed the gha-migration-3 branch from c05fff2 to a00ea5f Compare October 20, 2020 15:07

pull-request-size bot added size/L and removed size/M labels Oct 20, 2020

AzfaarQureshi force-pushed the gha-migration-3 branch from a00ea5f to 5adbc9d Compare October 20, 2020 15:25

shovnik mentioned this pull request Oct 20, 2020

Migrate CircleCI workflows to GitHub Actions (2/3) #3341

Merged

3 tasks

pracucci approved these changes Oct 21, 2020

View reviewed changes

pstibrany approved these changes Oct 21, 2020

View reviewed changes

Added deploy and deploy-website jobs, rebased and addressed requested…

91500cf

… changes Signed-off-by: Shovnik Bhattacharya <shovnik@amazon.com> adding comment explaining ssh in web_deploy job Signed-off-by: Azfaar Qureshi <azfaarq@amazon.com>

AzfaarQureshi force-pushed the gha-migration-3 branch from 5adbc9d to 91500cf Compare October 21, 2020 23:35

pstibrany merged commit 2936ea9 into cortexproject:master Oct 22, 2020

pracucci mentioned this pull request Oct 22, 2020

Fixed deploy via GitHub actions #3380

Merged

3 tasks

		SSH_AUTH_SOCK: /tmp/ssh_agent.sock
		GIT_SSH_COMMAND: "ssh -o StrictHostKeyChecking=no"

	- name: Setup SSH Keys and known_hosts
	- name: Setup SSH Keys and known_hosts used to authenticated to GitHub to deploy website

		QUAY_PASSWORD: ${{secrets.QUAY_PASSWORD}}
		QUAY_USER: ${{secrets.QUAY_USER}}

Migrate CircleCI workflows to GitHub Actions (3/3) #3368

Migrate CircleCI workflows to GitHub Actions (3/3) #3368

Uh oh!

Conversation

AzfaarQureshi commented Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Which issue(s) this PR fixes:

Checklist

Uh oh!

pracucci commented Oct 20, 2020

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shovnik commented Oct 20, 2020

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

shovnik commented Oct 21, 2020

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AzfaarQureshi Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AzfaarQureshi commented Oct 19, 2020 •

edited

Loading

pracucci Oct 20, 2020 •

edited

Loading

pracucci Oct 20, 2020 •

edited

Loading

pracucci commented Oct 20, 2020 •

edited

Loading

AzfaarQureshi Oct 21, 2020 •

edited

Loading