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

Move from Travis CI to github actions #698

Merged
merged 4 commits into from
Nov 6, 2020
Merged

Move from Travis CI to github actions #698

merged 4 commits into from
Nov 6, 2020

Conversation

akosveres
Copy link
Contributor

@akosveres akosveres commented Nov 4, 2020

Description

The pull request has the following changes:

  • github actions workflow following the travis ci build
  • update README with new github actions badge instead of the travis ci one
  • remove .travis.yaml
  • remove now unneeded bash script which sets travis ci experimental flag

Motivation and Context

How Has This Been Tested?

The workflow has been tested on my private fork, all steps have completed successfully, except the push to github container registry.

Link to latest build https://github.com/akosveres/faas-netes/runs/1354594029?check_suite_focus=true

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.

Signed-off-by: Akos Veres <veres@akos.me>
@derek derek bot added the new-contributor label Nov 4, 2020
Signed-off-by: Akos Veres <veres@akos.me>
@akosveres
Copy link
Contributor Author

Additional question is: do we want to push images when tags get released? That would be new functionality and I was not sure if it's something we want to add now or as a separate PR?

@alexellis
Copy link
Member

Yes definitely and that shouldn't be new behaviour as we do that now with Travis.

Signed-off-by: Akos Veres <veres@akos.me>
@akosveres
Copy link
Contributor Author

akosveres commented Nov 4, 2020

More questions:

  • When do we want to push the latest tag? Maybe we need a separate flow for when merging in to master and only then pushing the latest tag and not during the PR.
  • Do we want to test the code during a tag push or would it be acceptable to just build the container and then push it to the registry?

Actions on the repository owners side to make this PR pass:

  • Enable github actions for the repository
  • Add docker secrets, we have to agree on the naming of the secrets.

@alexellis
Copy link
Member

When do we want to push the latest tag? Maybe we need a separate flow for when merging in to master and only then pushing the latest tag and not during the PR.

Upon release tags along with the tag itself.

Do we want to test the code during a tag push or would it be acceptable to just build the container and then push it to the registry?

I think it is acceptable to only run e2e tests in a "ci-only" run (git push/PR) and to just release images in the "publish" job. This is the strategy I took with the example I shared on yesterday's call. https://github.com/alexellis/registry-creds/tree/master/.github/workflows

Actions on the repository owners side to make this PR pass:
Enable github actions for the repository
Add docker secrets, we have to agree on the naming of the secrets.

Will do 👍

@@ -1,7 +1,7 @@
faas-netes - Kubernetes controller for OpenFaaS
===========

[![Build Status](https://travis-ci.com/openfaas/faas-netes.svg?branch=master)](https://travis-ci.com/openfaas/faas-netes)
[![Build Status](https://github.com/openfaas/faas-netes/workflows/build/badge.svg?branch=master)](https://github.com/openfaas/faas-netes/actions)
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I actually forgot about this.

@Waterdrips @LucasRoesler ☝️

@@ -0,0 +1,69 @@
name: publish
Copy link
Member

Choose a reason for hiding this comment

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

Let's call the file publish as per the reference examples.

run: ./contrib/run_function.sh
- name: stop dev cluster
run: ./contrib/stop_dev.sh
- name: Sleep for 10 seconds
Copy link
Member

Choose a reason for hiding this comment

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

Can you do an inline GitHub script instead if a wait is needed? Third-party actions are disabled.

Copy link
Member

Choose a reason for hiding this comment

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

- name: deploy deploy
run: OPERATOR=0 ./contrib/deploy.sh
- name: run function
run: ./contrib/run_function.sh
Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to OPERATOR=0 for consistency

uses: docker/build-push-action@v2
with:
outputs: "type=registry,push=true"
platforms: linux/amd64,linux/arm/v6,linux/arm64
Copy link
Member

Choose a reason for hiding this comment

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

We can do arm/v7 for Docker images, arm/v6 for published binaries

- name: Push containers
uses: docker/build-push-action@v2
with:
outputs: "type=registry,push=true"
Copy link
Member

Choose a reason for hiding this comment

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

I am conflicted about filling up the storage with images upon each commit into HEAD. Let's remove this for the time being.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If people want to try out solutions before their release via a tag, this might be beneficial. We could have a separate flow for pushing in to master for just this specifically, for the time being I'll remove the publishing of the images tho, as requested.

run: ./contrib/lint_chart.sh
- name: create cluster
run: ./contrib/create_cluster.sh
- name: deploy deploy
Copy link
Member

Choose a reason for hiding this comment

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

Let's only run these tests during the "push" / "pr" events?

Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Looks really good. Some comments were added for your input.

@akosveres
Copy link
Contributor Author

I believe I fixed most of the issues in the comments, feel free to look at it again.

* Run function with OPERATOR=0
* Use github-script instead of custom wait-action
* Remove push of container during CI run
* Rename tag file to publish
* Build armv7 images

Signed-off-by: Akos Veres <veres@akos.me>
- name: stop dev cluster
run: ./contrib/stop_dev.sh
- name: wait 10 seconds
run: sleep 10
Copy link
Member

Choose a reason for hiding this comment

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

Much better. I was thinking of this when out walking today.

Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

LGTM

@alexellis alexellis merged commit bf7fb6d into openfaas:master Nov 6, 2020
alexellis added a commit that referenced this pull request Nov 10, 2020
This was missed in #698

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
@alexellis alexellis mentioned this pull request Nov 10, 2020
alexellis added a commit that referenced this pull request Nov 10, 2020
This was missed in #698

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
alexellis added a commit that referenced this pull request Nov 10, 2020
This was missed in #698

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
@alexellis alexellis mentioned this pull request Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GitHub Actions Migration - tracking issue
2 participants