Migrate builds from Travis to Github Actions #839

utsavanand2 · 2020-11-06T16:34:26Z

Fixes #838

Issue #1585 for ref

Thanks to Lucas Roesler for his PR for buildx support #836
Signed-off-by: Utsav Anand [email protected]

Description

Motivation and Context

I have raised an issue to propose this change (required)

How Has This Been Tested?

Check out the ci-only build
and the publish build

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have signed-off my commits with git commit -s
I have added tests to cover my changes.
All new and existing tests passed.

alexellis · 2020-11-06T16:40:02Z

.github/workflows/ci-only.yaml

+        with:
+            fetch-depth: 1
+      -  
+        name: Get tags


IMHO The tag isn't required for builds / ci.

alexellis · 2020-11-06T16:40:27Z

.github/workflows/ci-only.yaml

+        name: Local docker build (root image)
+        uses: docker/build-push-action@v2
+        with:
+          context: .


Context + path can be dropped.

Isn't there a new Dockerfile for this like Dockerfile.nonroot?

OK you are controlling it via --target release here -> https://github.com/openfaas/faas-cli/blob/master/build.sh

@alexellis I'm controlling it with

with: context: . file: ./Dockerfile target: release

and

with: context: . file: ./Dockerfile target: root

alexellis · 2020-11-06T16:40:54Z

.github/workflows/ci-only.yaml

+        uses: docker/build-push-action@v2
+        with:
+          context: .
+          file: ./Dockerfile


Does one of the file: ./Dockerfile options need to be the root/non-root Dockerfile?

We can target just the non-root or the root docker image in CI, I just kept it here to be consistent with what we had in travis.yml

alexellis · 2020-11-06T16:41:27Z

.github/workflows/publish.yaml

+        env:
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+        with:
+          asset_paths: '["./faas-cli*"]'


I would put the binaries into ./bin/

alexellis · 2020-11-06T16:41:57Z

Closes #838 from @LucasRoesler

alexellis · 2020-11-06T16:42:27Z

.github/workflows/ci-only.yaml

+      - "*"
+  pull_request:
+    branches:
+      - "master"


Should this be on * also?

alexellis · 2020-11-06T16:43:18Z

.github/workflows/ci-only.yaml

+    runs-on: ubuntu-latest
+    steps:
+      - 
+        uses: actions/checkout@master


See the setup go action from openfaas/faas-netes#698

utsavanand2 · 2020-11-08T08:29:23Z

@alexellis I've resolved the above issues. Does this look like ready to be merged?

derek · 2020-11-15T16:57:03Z

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

LucasRoesler · 2020-11-15T18:52:28Z

.github/workflows/publish.yaml

+        with:
+          registry: ghcr.io
+          username: ${{ github.repository_owner }}
+          password: ${{ secrets.CR_PAT }}


why can't we use the default ${{ secrets.GITHUB_TOKEN }} i feel like we discussed it in the wednesday call, but I can't remember why

For the record: me and Lucas tried it and later found out it doesn't work for forked repos.
https://docs.github.com/en/free-pro-team@latest/actions/reference/authentication-in-a-workflow#permissions-for-the-github_token

So what is CR_PAT?

If it's not convention then can we use DOCKER_PASSWORD as per all the other merged and released builds?

Its a secret for Container Registry.
I have seen CR_PAT in the Github Docs, thought it would be better if we go with it. What do you think?
https://docs.github.com/en/free-pro-team@latest/packages/getting-started-with-github-container-registry/migrating-to-github-container-registry-for-docker-images#authenticating-with-the-container-registry

How faas-netes worked https://github.com/openfaas/faas-netes/tree/master/.github/workflows

is CR_PAT something that works without me doing something on each repo?

That link doesn't reference CR_PAT, is it the right link?

Sorry I can see where CR_PAT is mentioned now. It's not a special variable and has no meaning to Github Actions - can you change to DOCKER_PASSWORD please?

alexellis · 2020-11-16T19:09:21Z

.github/workflows/publish.yaml

+            REPO=${{ steps.get_repo_name.outputs.REPOSITORY_NAME }}
+          push: true
+          tags: |
+            ghcr.io/${{ steps.get_owner_name.outputs.OWNER_NAME }}/${{ steps.get_repo_name.outputs.REPOSITORY_NAME }}:${{ github.sha }}


I'm seeing a lot of duplication. Seems like this could be put into a similar variable and used again?

Do you mean something like an ENV VAR?

The same way you split the string in a step, construct the GHCR main image prefix once instead of 6+ times.

alexellis · 2020-11-16T19:09:43Z

Dockerfile

+ARG NS
+ARG REPO
+
+LABEL org.opencontainers.image.source https://github.com/$NS/$REPO


Does that string need to be split out?

No it doesn't need to be split out. I'll put a = in between.

Can we not inject the complete URL instead?

If you mean injecting the whole org.opencontainers.image.source=https://github.com/$REPO into the Dockerfile with build args, then I think it's not possible. The Dockerfile expects two args in LABEL, I get a red squiggly line saying so.

Okay I get it, by URL you mean this> #839 (comment)

alexellis · 2020-11-16T19:10:30Z

.github/workflows/build.yaml

+          build-args: |
+            VERSION=latest-dev
+            GIT_COMMIT=${{ github.sha }}
+            NS=openfaas


Can't this be a single variable of GITHUB_REPOSITORY?

Or whatever is the full path to the repo to inject in the special label?

Yes we can get the GITHUB_URL into the build args. Sure!

alexellis · 2020-11-16T19:19:30Z

.github/workflows/build.yaml

+          build-args: |
+            VERSION=latest-dev
+            GIT_COMMIT=${{ github.sha }}
+            NS=openfaas


Same comment as above

REPO_URL=https://github.com/$GITHUB_REPOSITORY

Thanks a lot!

alexellis

Changed requested please. What do you say to aiming to have everything ready for a merge on Weds?

Fixes openfaas#838 Thanks to Lucas Roesler for his PR for buildx support openfaas#836 Signed-off-by: Utsav Anand <[email protected]>

alexellis

Your IMAGE_PREFIX is much easier to follow than the duplicated strings. Thank you for working on this.

alexellis · 2020-11-17T14:46:16Z

.github/workflows/build.yaml

+        name: Set up Docker Buildx
+        uses: docker/setup-buildx-action@v1
+      - 
+        name: Local docker build (non-root image)


Do this in a follow-up PR if you want, but we need to do a multi-arch build upon every commit, not just upon a release tag. This found issues in the past.

I'll try to put up a PR by tonight to address this 👆

@alexellis I've resolved it in a PR I've just put up
#843

alexellis reviewed Nov 6, 2020

View reviewed changes

.github/workflows/ci-only.yaml Outdated

- "*"

pull_request:

branches:

- "master"

Copy link

Member

alexellis Nov 6, 2020

Choose a reason for hiding this comment

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

Should this be on * also?

alexellis reviewed Nov 6, 2020

View reviewed changes

utsavanand2 force-pushed the github-actions branch 5 times, most recently from 72ce897 to 3e2d7e9 Compare November 8, 2020 08:25

utsavanand2 force-pushed the github-actions branch 7 times, most recently from 1074f36 to 87f9b74 Compare November 11, 2020 20:03

derek bot added the no-dco label Nov 15, 2020

utsavanand2 force-pushed the github-actions branch 3 times, most recently from b6b293f to 29f69fb Compare November 15, 2020 17:14

LucasRoesler reviewed Nov 15, 2020

View reviewed changes

utsavanand2 force-pushed the github-actions branch from 29f69fb to 4ef030c Compare November 15, 2020 20:04

derek bot removed the no-dco label Nov 15, 2020

utsavanand2 force-pushed the github-actions branch from 4ef030c to 7d6213a Compare November 15, 2020 20:07

alexellis reviewed Nov 16, 2020

View reviewed changes

alexellis requested changes Nov 16, 2020

View reviewed changes

utsavanand2 force-pushed the github-actions branch 3 times, most recently from eebbaa9 to bc88dc9 Compare November 17, 2020 08:53

Migrate builds from Travis to Github Actions

25e83de

Fixes openfaas#838 Thanks to Lucas Roesler for his PR for buildx support openfaas#836 Signed-off-by: Utsav Anand <[email protected]>

utsavanand2 force-pushed the github-actions branch from bc88dc9 to 25e83de Compare November 17, 2020 12:29

alexellis approved these changes Nov 17, 2020

View reviewed changes

alexellis reviewed Nov 17, 2020

View reviewed changes

alexellis merged commit 15cc71a into openfaas:master Nov 17, 2020

Migrate builds from Travis to Github Actions #839

Migrate builds from Travis to Github Actions #839

Conversation

utsavanand2 commented Nov 6, 2020 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

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

alexellis commented Nov 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utsavanand2 commented Nov 8, 2020

derek bot commented Nov 15, 2020

Choose a reason for hiding this comment

utsavanand2 Nov 15, 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

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

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

alexellis left a comment

Choose a reason for hiding this comment

alexellis 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

utsavanand2 commented Nov 6, 2020 •

edited

Loading

utsavanand2 Nov 15, 2020 •

edited

Loading