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

AzfaarQureshi · 2020-10-08T16:43:40Z

What this PR does

This is PR 1/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]

AzfaarQureshi · 2020-10-08T16:48:26Z

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

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


A significant number of commands in the Makefile were hardcoded with an assumed file structure of the CI container. make sure previous commands don’t break a symlink was created instead from the hardcoded “expected” working directory go/src/github.com/cortexproject/cortex to the actual working directory $GITHUB_WORKSPACE.

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

AzfaarQureshi · 2020-10-08T16:49:20Z

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

+      - name: Zip Images
+        run: tar -zcvf images.tar.gz /tmp/images
+      - name: Upload Images Artifact
+        uses: actions/upload-artifact@v2
+        with:
+          name: Docker Images
+          path: ./images.tar.gz


As of Oct 2020 GitHub Actions does not persist between different jobs in the same workflow. Each job is run on a fresh virtual environment. As such, we need to upload and download artifacts to share data between jobs.

Docker Images are zipped before uploading as a workaround. The images contain characters that are illegal in the upload-artifact action.

Signed-off-by: Azfaar Qureshi <azfaarq@amazon.com>

pracucci

Very good job! Overall LGTM. I just left few questions here and there, but I can't see any blocker.

.github/README.md

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

pracucci · 2020-10-09T16:28:28Z

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

+  lint:
+    runs-on: ubuntu-latest
+    container:
+      image: quay.io/cortexproject/build-image:update-golang-1.14.9-eb0c8d4d2


Do you think we can use something similar to defaults: &defaults in the CircleCI yaml, in order to avoid the repetition of image: quay.io/cortexproject/build-image:update-golang-1.14.9-eb0c8d4d2 and being able to define the build image only once in this file?

There exists a "defaults" field for GitHub actions, but it automatically applies to all jobs in the workflow. The integration and integration-configs-db jobs that will be added to the workflow with PR 2/3 of the migration do not use the default container so using a default would require explicitly removing the default container from those jobs by setting them to None.

We would like to reuse setup commands and default containers with something like commands in CircleCI, but this feature is not available yet on GitHub Actions.

Status: https://github.community/t/reusing-sharing-inheriting-steps-between-jobs-declarations/16851

GHA doesn't support yaml anchors yet because of some security reasons but they are actively working on it

Would it be possible to use env variable instead?

This is not a blocker for getting this PR merged. We can discuss it separately.

pracucci · 2020-10-09T16:31:48Z

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

+      - uses: actions/upload-artifact@v2
+        with:
+          name: website public
+          path: website/public/
+      - uses: actions/upload-artifact@v2
+        with:
+          name: Frontend Protobuf
+          path: pkg/querier/frontend/frontend.pb.go
+      - uses: actions/upload-artifact@v2
+        with:
+          name: Caching Index Client Protobuf
+          path: pkg/chunk/storage/caching_index_client.pb.go
+      - uses: actions/upload-artifact@v2
+        with:
+          name: Ring Protobuf
+          path: pkg/ring/ring.pb.go
+      - uses: actions/upload-artifact@v2
+        with:
+          name: Cortex Protobuf
+          path: pkg/ingester/client/cortex.pb.go
+      - uses: actions/upload-artifact@v2
+        with:
+          name: Rules Protobuf
+          path: pkg/ruler/rules/rules.pb.go


Have you figured out if we actually need to upload these artifacts? I know we do it in CircleCI but I have the feeling they're unused (at least I don't remember if we still use them anywhere).

Oh, we included it because this PR here made it seem like there was an explicit decision to save these files. Has the situation changed? But you are right, those artifacts are not used anywhere else within CI

Since then we have added these files into Git, so I think #1224 is not relevant anymore.

I also think we don't need to store these artifacts anymore.

Signed-off-by: Azfaar Qureshi <azfaarq@amazon.com>

pstibrany

Great job, thank you very much for doing this!

(Nit: In next PRs I would suggest moving the comments from README.md into YAML files, to keep it at one place.)

pstibrany · 2020-10-12T08:37:05Z

.github/README.md

+- name: Sym link expected path to github workspace
+  run: |
+    mkdir -p /go/src/github.com/cortexproject/cortex
+    ln -s $GITHUB_WORKSPACE/* /go/src/github.com/cortexproject/cortex


This looks suspicious... why create many links instead of one:

mkdir -p /go/src/github.com/cortexproject/ ln -s "$GITHUB_WORKSPACE" /go/src/github.com/cortexproject/cortex

pstibrany · 2020-10-12T08:41:36Z

.github/README.md

+*Note:* Docker Images are zipped before uploading as a workaround. The images contain characters that are illegal in the upload-artifact action.
+```yaml
+- name: Compressing Images
+        run: tar -zcvf images.tar.gz /tmp/images


Not sure if compression gives us anything here. We can use tar without -z flag to make it run faster (but use more disk space).

The compression was in fact unnecessary. We removed it in the subsequent migration PR: #3341

pstibrany · 2020-10-12T08:46:04Z

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

+    steps:
+      - name: Checkout Repo
+        uses: actions/checkout@v1
+      - name: Sym Link Expected Path to Workspace


This linking is explained in README, apart from doing review, who reads that? I'd include the explanations from README directly into this file.

Added a comment explaining this within the code itself in the subsequent PR to address this.
#3341

pstibrany · 2020-10-12T08:48:42Z

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

+  lint:
+    runs-on: ubuntu-latest
+    container:
+      image: quay.io/cortexproject/build-image:update-golang-1.14.9-eb0c8d4d2


Would it be possible to use env variable instead?

pstibrany · 2020-10-12T08:49:19Z

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

+          mkdir -p /go/src/github.com/cortexproject/cortex
+          ln -s $GITHUB_WORKSPACE/* /go/src/github.com/cortexproject/cortex
+      - name: Lint
+        run: make BUILD_IN_CONTAINER=false lint


Should we set BUILD_IN_CONTAINER=false as env variable for entire workflow or at least individual jobs?

But BUILD_IN_CONTAINER is not an environment variable. At least, doesn't work if you set it as env variable. Am I missing anything?

pstibrany · 2020-10-12T08:53:25Z

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

+      - uses: actions/upload-artifact@v2
+        with:
+          name: website public
+          path: website/public/
+      - uses: actions/upload-artifact@v2
+        with:
+          name: Frontend Protobuf
+          path: pkg/querier/frontend/frontend.pb.go
+      - uses: actions/upload-artifact@v2
+        with:
+          name: Caching Index Client Protobuf
+          path: pkg/chunk/storage/caching_index_client.pb.go
+      - uses: actions/upload-artifact@v2
+        with:
+          name: Ring Protobuf
+          path: pkg/ring/ring.pb.go
+      - uses: actions/upload-artifact@v2
+        with:
+          name: Cortex Protobuf
+          path: pkg/ingester/client/cortex.pb.go
+      - uses: actions/upload-artifact@v2
+        with:
+          name: Rules Protobuf
+          path: pkg/ruler/rules/rules.pb.go


Since then we have added these files into Git, so I think #1224 is not relevant anymore.

pracucci

LGTM, thanks!

pracucci · 2020-10-12T09:27:49Z

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

+          mkdir -p /go/src/github.com/cortexproject/cortex
+          ln -s $GITHUB_WORKSPACE/* /go/src/github.com/cortexproject/cortex
+      - name: Lint
+        run: make BUILD_IN_CONTAINER=false lint


But BUILD_IN_CONTAINER is not an environment variable. At least, doesn't work if you set it as env variable. Am I missing anything?

pracucci · 2020-10-12T09:31:08Z

@AzfaarQureshi We merged the PR since it's in a good shape, but we would be glad if you could address the remaining comments in a subsequent PR. Thanks!

pstibrany · 2020-10-12T09:35:49Z

But BUILD_IN_CONTAINER is not an environment variable. At least, doesn't work if you set it as env variable. Am I missing anything?

"Every environment variable that make sees when it starts up is transformed into a make variable with the same name and value." (from GNU Make docs)

But you say that it doesn't work when set as env variable, so perhaps there is something in our Makefile that prevents that?

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

shovnik force-pushed the gha-migration-1 branch from e5752ef to 0542f3f Compare October 8, 2020 16:47

AzfaarQureshi commented Oct 8, 2020

View reviewed changes

shovnik force-pushed the gha-migration-1 branch 2 times, most recently from d383cf7 to 19f90c4 Compare October 8, 2020 17:33

AzfaarQureshi force-pushed the gha-migration-1 branch 2 times, most recently from bb100e5 to df057f0 Compare October 8, 2020 18:21

shovnik and others added 3 commits October 8, 2020 14:22

Added lint job

ae58efa

Signed-off-by: Azfaar Qureshi <azfaarq@amazon.com>

Added test and build jobs

38ae76c

Signed-off-by: Azfaar Qureshi <azfaarq@amazon.com>

adding README

f96681c

Signed-off-by: Azfaar Qureshi <azfaarq@amazon.com>

AzfaarQureshi force-pushed the gha-migration-1 branch from df057f0 to f96681c Compare October 8, 2020 18:22

pracucci approved these changes Oct 9, 2020

View reviewed changes

adding table to README and removing vestigial lines from workflow

8319961

Signed-off-by: Azfaar Qureshi <azfaarq@amazon.com>

pstibrany approved these changes Oct 12, 2020

View reviewed changes

pracucci mentioned this pull request Oct 12, 2020

Actions #3230

Closed

3 tasks

pracucci approved these changes Oct 12, 2020

View reviewed changes

pracucci merged commit 491e607 into cortexproject:master Oct 12, 2020

This was referenced Oct 12, 2020

Upgrade build-image from Debian stretch to buster #3323

Merged

Remove artifacts upload from GitHub actions #3324

Merged

pracucci mentioned this pull request Oct 13, 2020

Unit tests in GitHub actions workflow are not running with -race #3337

Closed

shovnik mentioned this pull request Oct 13, 2020

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

Merged

3 tasks

AzfaarQureshi mentioned this pull request Oct 20, 2020

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

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AzfaarQureshi commented Oct 8, 2020 •

edited

Loading

AzfaarQureshi Oct 8, 2020

AzfaarQureshi Oct 8, 2020

pracucci left a comment

pracucci Oct 9, 2020

shovnik Oct 9, 2020 •

edited

Loading

AzfaarQureshi Oct 9, 2020 •

edited

Loading

pstibrany Oct 12, 2020

pracucci Oct 12, 2020 •

edited

Loading

pracucci Oct 9, 2020

AzfaarQureshi Oct 9, 2020 •

edited

Loading

pstibrany Oct 12, 2020

pstibrany Oct 12, 2020

pracucci Oct 12, 2020

pstibrany left a comment

pstibrany Oct 12, 2020

pstibrany Oct 12, 2020

shovnik Oct 13, 2020

pstibrany Oct 12, 2020

shovnik Oct 13, 2020

pstibrany Oct 12, 2020

pstibrany Oct 12, 2020

pracucci Oct 12, 2020

pstibrany Oct 12, 2020

pracucci left a comment

pracucci Oct 12, 2020

pracucci commented Oct 12, 2020

pstibrany commented Oct 12, 2020 •

edited

Loading

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

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

Conversation

AzfaarQureshi commented Oct 8, 2020 • edited Loading

What this PR does

Which issue(s) this PR fixes:

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shovnik Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

AzfaarQureshi Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AzfaarQureshi Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci commented Oct 12, 2020

pstibrany commented Oct 12, 2020 • edited Loading

AzfaarQureshi commented Oct 8, 2020 •

edited

Loading

shovnik Oct 9, 2020 •

edited

Loading

AzfaarQureshi Oct 9, 2020 •

edited

Loading

pracucci Oct 12, 2020 •

edited

Loading

AzfaarQureshi Oct 9, 2020 •

edited

Loading

pstibrany commented Oct 12, 2020 •

edited

Loading