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

GitHub Actions Migration - tracking issue #1585

Closed
14 of 28 tasks
Waterdrips opened this issue Nov 4, 2020 · 12 comments · Fixed by openfaas/faas-netes#698, openfaas/faasd#125 or openfaas/store#133
Closed
14 of 28 tasks

Comments

@Waterdrips
Copy link
Contributor

Waterdrips commented Nov 4, 2020

Inlets

alexellis

openfaas

Merged:

Waiting on Review:

Pending:

OpenFaaS Incubator

@Waterdrips
Copy link
Contributor Author

/open

@Waterdrips
Copy link
Contributor Author

/reopen

@alexellis
Copy link
Member

alexellis commented Nov 11, 2020

Folks unfortunately the first set of merged PRs did not work as I don't believe that we tested them in our branches with a release tag before submitting the PR and asking for a merge. Let's tighten up our work before asking for a merge?

Make sure release tags work

I also noticed that the Docker v2 build/push action does not copy in a .git folder, this can be worked-around as follows (with a path and context). When these are missing, the code does a fresh clone / checkout of the code and lacks the .git folder used to get versions and tags.

      - name: Build multi-arch containers for validation only
        uses: docker/build-push-action@v2
        with:
          context: .
          file: ./Dockerfile
          outputs: "type=image,push=false"
          platforms: linux/amd64,linux/arm/v7,linux/arm64
          tags: |
            ghcr.io/openfaas/faas-netes:${{ github.sha }}

Include a NS variable in the image name

In order to test in our own repos, we should each have some way of overwriting ghcr.io/openfaas/faas-netes:${{ github.sha }} with our own username i.e. ghcr.io/NS/faas-netes:${{ github.sha }} where NS is a GitHub Action env-var. Have a look at https://docs.github.com/en/free-pro-team@latest/actions/reference/environment-variables

Keep naming consistent

I noticed that in most PRs people changed the name of the build and publish step to whatever they preferred. Please don't. Let's make sure this is consistent for future maintenance. Match the build job with the filename too.

Build on *

Many PRs just assumed only master would be used

on:
  push:
    branches:
    - '*'
  pull_request:
    branches:
    - '*'
  • build.yaml - upon * for push and branches
  • publish.yaml - upon tags

Make sure this is multi-arch with buildx

We need to build for multiple architectures, not just x86_64 - see the faas-netes build or the other references examples on trello.

Remove .travis.yml and travis_terminate

Remove the .travis.yml file

Remove the travis_terminate command in bash files.

Make sure the Go version is set when required

jobs:
  build:
    strategy:
      matrix:
        go-version: [1.13.x]
        os: [ubuntu-latest]
    runs-on: ${{ matrix.os }}
    steps:
      - uses: actions/checkout@master
        with:
          fetch-depth: 1
      - name: Install Go
        uses: actions/setup-go@v2
        with:
          go-version: ${{ matrix.go-version }}

Some people set the Go version to 1.11, 1.9 and 1.14 - for the time being we're using 1.13, but only configure the Go version if Go is used in the actions. For most of the openfaas repos Go isn't used at all at build-time, Docker is.

Link the artifact to the repo

Screenshot 2020-11-11 at 16 57 00

Waterdrips added a commit to Waterdrips/ofc-bootstrap that referenced this issue Nov 11, 2020
This changes the name of "ci-only" action to build as per comments in
openfaas/faas#1585. Also pins go version
based on the same

Signed-off-by: Alistair Hey <[email protected]>
Waterdrips added a commit to Waterdrips/ofc-bootstrap that referenced this issue Nov 11, 2020
This changes the name of "ci-only" action to build as per comments in
openfaas/faas#1585. Also pins go version
based on the same

Signed-off-by: Alistair Hey <[email protected]>
Waterdrips added a commit to Waterdrips/ofc-bootstrap that referenced this issue Nov 11, 2020
This changes the name of "ci-only" action to build as per comments in
openfaas/faas#1585. Also pins go version
based on the same

Signed-off-by: Alistair Hey <[email protected]>
@Waterdrips
Copy link
Contributor Author

Waterdrips commented Nov 12, 2020

fyi you can get the repo name like this in an action

      - name: Get Repo
        id: get_repo
        run: echo ::set-output name=repo_owner::$(echo ${{ github.repository_owner }} | tr '[:upper:]' '[:lower:]')

then ref it with this

            ghcr.io/${{ steps.get_repo.outputs.repo_owner }}/${{ matrix.svc }}:latest

then users use ${{ github.repository_owner }} as the "DOCKER_USERNAME" for the docker login, rather than needing to set a secret to allow pusing to a user's ghcr repo while testing.

(edit, changed based on conversations below, so may not make sense in those convos now)

@CodeCutterUK
Copy link

@Waterdrips Yes but the catch is event isn't always included from what I've read, although that might only be for scheduled triggers....it's probably fine in the context of openfaas builds though, but not really my call.

@Waterdrips
Copy link
Contributor Author

it looks like its only scheduled builds that dont have events from the docs and forums, we dont use those currently.

@CodeCutterUK
Copy link

So using this instead of github.repository_owner or the currently undocumented env GITHUB_REPOSITORY_OWNER results in the same issue if the repo owner string is storied in a secret, which is highly likely given that the GHCR username would like be stored as a secret. Again likely not an issue for repos in a 'proper' org like, but would likely be an issue for forks thus not fully solving the issue we wanted to solve.

You can see the issue here: https://github.com/CodeCutterUK/nats-connector/runs/1395097635?check_suite_focus=true
versus when the secrets are removed: https://github.com/CodeCutterUK/nats-connector/runs/1395105402?check_suite_focus=true

@Waterdrips
Copy link
Contributor Author

I never found ${{ github.repository_owner }} - i have found the GH Actions docs to be not amazing... I like that more than using the event...

/so as far as i can tell the problem is:

  • images need all lowrcase repos.
  • GHCR login needs your actual github username (With correct case)

so we need both the explicit docker username, or use the non-lowercased repo owner (however we decide to get it) - need to decide how we manage this, we could use the ${{ github.repository_owner }} as the dockerhub login, and have a step that gets the lowercase version for image tags, repo path etc?

@Waterdrips
Copy link
Contributor Author

Waterdrips commented Nov 13, 2020

@CodeCutterUK updated based on the above:https://github.com/Waterdrips/openfaas-cloud/actions/runs/361500170 seems to work well without the secret existing

@CodeCutterUK
Copy link

CodeCutterUK commented Nov 13, 2020

Yes, works perfectly without the lowercase version of the username as a secret :)
All a bit annoying really, I'm not sure there will be a work around without GH changing some code

alexellis pushed a commit to openfaas/ofc-bootstrap that referenced this issue Nov 18, 2020
This changes the name of "ci-only" action to build as per comments in
openfaas/faas#1585. Also pins go version
based on the same

Signed-off-by: Alistair Hey <[email protected]>
@developer-guy
Copy link
Contributor

I completed the Github Action migration issue for the connector-sdk, here is the link below:
openfaas/connector-sdk#53

@alexellis
Copy link
Member

alexellis commented Apr 7, 2021

Thanks to @cpanato who has volunteered to pick up the NATS queue worker. This will need a new PR, and I'd suggest using one of the other projects in this list as a template. For instance: https://github.com/openfaas/cron-connector

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