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

shovnik · 2020-10-13T21:33:42Z

What this PR does

This is PR 2/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
Addresses comments on PR 1/3: removing compression for tar and removing useless artifacts from README

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

cc - @alolita

Which issue(s) this PR fixes:

fixes a part of #3274

Checklist

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

…rkflow Signed-off-by: Azfaar Qureshi <[email protected]> Signed-off-by: Shovnik Bhattacharya <[email protected]> adding table to README and removing vestigial lines from workflow Signed-off-by: Azfaar Qureshi <[email protected]> Added integration and integration-configs-db jobs Signed-off-by: Azfaar Qureshi <[email protected]> reading from cortexproject Signed-off-by: Azfaar Qureshi <[email protected]> Made Step Naming Consistent Signed-off-by: Shovnik Bhattacharya <[email protected]> read tag only if they exist on the push event updating quay image removing old README changing to v2 Addressed changes requested in PR 1/3

shovnik · 2020-10-14T14:53:08Z

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

+      - name: Sym Link Expected Path to Workspace
+        run: |
+          sudo mkdir -p /go/src/github.com/cortexproject/cortex
+          sudo ln -s $GITHUB_WORKSPACE/* /go/src/github.com/cortexproject/cortex


Linking the directory instead of the contents of the folder did not work when we tested it.

AzfaarQureshi · 2020-10-14T15:22:02Z

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

+          echo "Running integration tests with image: $CORTEX_IMAGE"
+          go test -tags=requires_docker -timeout 1200s -v -count=1 ./integration/...
+        env:
+          IMAGE_PREFIX: ${{ secrets.IMAGE_PREFIX }}


This secret will need to be added in the forked repo if one wants to change the image_prefix

pracucci

Good job on this 2nd part too! 👏 I left few comments and then we should be good to go 🚀

pracucci · 2020-10-15T16:13:41Z

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

-        run: tar -zcvf images.tar.gz /tmp/images
-      - name: Upload Images Artifact
+      - name: Create Docker Images Archive
+        run: tar -cvf images.tar.gz /tmp/images


Given we're not compressing anymore, could you remove the .gz suffix?

pracucci · 2020-10-15T16:20:49Z

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

+    needs: build
+    runs-on: ubuntu-16.04
+    steps:
+      - name: Install Docker Client


This is also done in build. Is there any way to build a shared step? Just asking, not a blocker.

We could replace this with a YAML anchor (which GitHub Actions has plans to release shortly) or we could write a custom action and use that instead. With the custom action we would replace the Install Docker Client step with something like this in both Build and Integration.

steps: - name: Install Docker Client uses: cortexproject/install-docker-client - run: ...

This means you'll have to maintain another actions repo and that might bring unwanted toil when upgrading versions. But it might be worth if you have a lot of boilerplate set-up.

A simple way to have this logic in one place would be to create an install-docker.sh script and call it whenever an image requires a clean docker client installation. This would also make it easier to update the docker version in a single place. Is that close to what you had in mind?

^ This is a better idea imo. This way you won't need to update the docker client version manually every time the install docker client step is called. The script can simply read $VER which can be set as a global variable for the entire workflow at the top of the file.

I committed the solution using an install-docker script. Let me know if it is okay. @pracucci

pracucci · 2020-10-15T16:22:06Z

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

+        uses: actions/download-artifact@v2
+        with:
+          name: Docker Images
+      - name: Create Docker Images Archive


I guess you mean "Extract" instead of "Create"

Slight brain fart 😅 , thanks

pracucci · 2020-10-15T16:22:44Z

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

+          docker pull quay.io/cortexproject/cortex:v0.6.0
+          docker pull quay.io/cortexproject/cortex:v0.7.0


Could you remove this, please? Not required anymore.

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

pracucci · 2020-10-15T16:24:42Z

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

+          export CORTEX_IMAGE="${CORTEX_IMAGE_PREFIX}cortex:${TAG:-$(./tools/image-tag)}"
+          export CORTEX_CHECKOUT_DIR="/go/src/github.com/cortexproject/cortex"
+          echo "Running integration tests with image: $CORTEX_IMAGE"
+          go test -tags=requires_docker -timeout 1200s -v -count=1 ./integration/...


I recently increased this timeout to 1800s. Could you update it, please?

pracucci · 2020-10-15T16:28:13Z

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

+        run: |
+          touch build-image/.uptodate
+          MIGRATIONS_DIR=$(pwd)/cmd/cortex/migrations
+            make BUILD_IMAGE=quay.io/cortexproject/build-image:upgrade-build-image-debian-491e60715-WIP TTY='' configs-integration-test


Could you add a comment to explain why we need TTY='', please?

The default runners in GitHub Actions don't support TTY :( actions/runner#241 and a GitHub member responded saying its fairly low prio in their backlog.

This is what happens when we remove TTY='':

However, there is a workaround we can do if you'd like it back: https://man7.org/linux/man-pages/man1/script.1.html
(script -e -c $COMMAND_HERE)
The snippet would be changed to:

steps: - uses: actions/checkout@v1 run: | touch build-image/.uptodate MIGRATIONS_DIR=$(pwd)/cmd/cortex/migrations script -e -c "make BUILD_IMAGE=quay.io/cortexproject/build-image:upgrade-build-image-debian-491e60715-WIP configs-integration-test"

This works and enables the command to run with TTY but this seemed kind of hacky to me so I left it as TTY=''

Would you like us to change the line to use script -e -c or just add a comment saying GHA doesnt support TTY in their default runners yet

I think comment explaining why TTY='' is fine.

just add a comment saying GHA doesnt support TTY in their default runners yet

Let's just add a comment, but not a block so you can do it in a following PR.

Signed-off-by: Shovnik Bhattacharya <[email protected]>

shovnik · 2020-10-16T20:31:05Z

Hey @pracucci , we addressed all the changes requested in the comments on the PR. Is this PR in a state ready to be merged?

pstibrany

Thanks for your work!

pracucci

LGTM, thanks!

pracucci · 2020-10-20T07:04:01Z

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

+        run: |
+          touch build-image/.uptodate
+          MIGRATIONS_DIR=$(pwd)/cmd/cortex/migrations
+            make BUILD_IMAGE=quay.io/cortexproject/build-image:upgrade-build-image-debian-491e60715-WIP TTY='' configs-integration-test


just add a comment saying GHA doesnt support TTY in their default runners yet

Let's just add a comment, but not a block so you can do it in a following PR.

…rgid-ctx * 'master' of github.com:cortexproject/cortex: Enforce integration tests default flags config to never be overwritten (cortexproject#3370) Avoid deletion of blocks which are not shipped (cortexproject#3346) Upgrade Thanos to latest master (cortexproject#3363) Migrate CircleCI workflows to GitHub Actions (2/3) (cortexproject#3341) Remove comments that doesn't seem right (cortexproject#3361) add ingester interface (cortexproject#3352) Fail fast an ingester if unable to load existing TSDBs (cortexproject#3354) Fixed Gossip memberlist members joining when addresses are configured using DNS-based service discovery (cortexproject#3360) Export distributor method to get ingester replication set (cortexproject#3356) Correct link for Block Storage reference (cortexproject#3234) Added section on Cleaner. (cortexproject#3327) Update prometheus vendor to master (cortexproject#3345) adding GHA CI env variable check (cortexproject#3351) Add ingesters shuffle sharding support on the read path (cortexproject#3252)

pull-request-size bot added the size/M label Oct 13, 2020

shovnik mentioned this pull request Oct 13, 2020

Migrate CircleCI workflows to GitHub Actions (1/3) #3302

Merged

3 tasks

shovnik commented Oct 14, 2020

View reviewed changes

AzfaarQureshi reviewed Oct 14, 2020

View reviewed changes

pracucci reviewed Oct 15, 2020

View reviewed changes

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

Made changes suggested by pracucci in review

778797e

Signed-off-by: Shovnik Bhattacharya <[email protected]>

shovnik force-pushed the gha-migration-2 branch 5 times, most recently from 226f5d4 to fce62df Compare October 15, 2020 18:54

Moved docker installation to script

5934d49

Signed-off-by: Shovnik Bhattacharya <[email protected]>

shovnik force-pushed the gha-migration-2 branch from fce62df to 5934d49 Compare October 15, 2020 19:03

AzfaarQureshi mentioned this pull request Oct 19, 2020

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

Merged

3 tasks

pstibrany approved these changes Oct 20, 2020

View reviewed changes

pracucci approved these changes Oct 20, 2020

View reviewed changes

pracucci merged commit be0606e into cortexproject:master Oct 20, 2020

		docker pull quay.io/cortexproject/cortex:v0.6.0
		docker pull quay.io/cortexproject/cortex:v0.7.0

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

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

Uh oh!

Conversation

shovnik commented Oct 13, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

AzfaarQureshi Oct 15, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

pstibrany 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!

shovnik commented Oct 16, 2020

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shovnik commented Oct 13, 2020 •

edited

Loading

AzfaarQureshi Oct 15, 2020 •

edited

Loading

pstibrany Oct 20, 2020 •

edited

Loading