tinkerbell / tink

Workflow Engine for provisioning Bare Metal
https://tinkerbell.org
Apache License 2.0
927 stars 134 forks source link

From Drone to Github Action #233

Closed gianarb closed 4 years ago

gianarb commented 4 years ago

In the ongoing effort to release the pressure to our internal drone.io and to improve independency for Tinkerbell as a project, we should move our CI/CD to GitHub Actions.

I had a look at it, but I would like to discuss here how to re-do it to see if we can make some improvements along the way.

What does it currently do to commit and pull request:

  1. go test
  2. runs ./ci-checks.sh
  3. build docker images
  4. go import, golint
  5. push cli docker image
  6. push tink server docker image
  7. push tink worker docker image

What I think we should do in the order:

  1. go fmt, go lint, goimport, #230 , go test
  2. runs ci-checks.sh

I am not against but I am not 100% sure we should push images for all the commits, it sounds a waste and confusing in our registry for community people coming and looking for a particular version. I think we should always build the images to see if everything works but we should push them only when a commit lands on the master branch using the latest tag maybe or unstable

if you agree with that I will proceed with the implementation:

THIS ISSUE IS NOT ABOUT RELEASE. It will have its own space

/cc. @mmlb @thebsdbox

thebsdbox commented 4 years ago

I agree with the push of images... things should be built internally if possible (or perhaps with a -devel) including all of the testing tags etc.. the actual push should only occur on an actual release..

gianarb commented 4 years ago

Can you elaborate @thebsdbox things should be built internally if possible? What does it mean internally? I presume a release will still be managed via GitHub action, but in its own way when a tag gets pushed.

displague commented 4 years ago

I am not against but I am not 100% sure we should push images for all the commits, it sounds a waste and confusing in our registry for community people coming and looking for a particular version. I think we should always build the images to see if everything works but we should push them only when a commit lands on the master branch using the latest tag maybe or unstable

This sounds reasonable to me. The images I would expect to find published would be "latest", any special long-lived branches, and tagged releases. Additional git-sha based images are noise.

IMHO, when tests are run for a PR, e2e tests should avoid publishing images and rely on pushing the image onto the test nodes directly. This avoids caching problems but also keeps our tests focused on the images themselves and not the image distribution system.

THIS ISSUE IS NOT ABOUT RELEASE. It will have its own space

I didn't read the warning before writing the following: E2E tests on 'latest' and tagged releases may benefit from using registry fetched images, but this creates a chicken-and-egg problem. Do you publish images before the E2E test, for the E2E test? Or do you wait for the E2E test to pass (using local copies of the images) before publishing them?

thebsdbox commented 4 years ago

Can you elaborate @thebsdbox things should be built internally if possible?

All testing and development images if possible should be pushed to some internal registry for testing so we can ideally do what ever tests we need to do without confusing what ever external registry we decide as our "source of truth" for published images.

mmlb commented 4 years ago

The image pushes go to a separate quay repo, thats true for all of our repos here. See https://github.com/tinkerbell/tink/blob/master/.drone.yml#L85 vs https://github.com/tinkerbell/tink/blob/master/.drone.yml#L112

I agree with the push of images... things should be built internally if possible (or perhaps with a -devel) including all of the testing tags etc.. the actual push should only occur on an actual release..

There are a couple of reasons we push PR images to a central repo.

  1. Allows us to make use of the freshly built images in the .drone.yml later on for docker-compose type integration tests. We aren't doing that in any of these public repos though (yet?).
  2. By pushing the PR images some other person can more easily test locally.

So we already do that basically, but with the added benefit of it being publicly available. I'd like to keep this.

We decided to split into a separate image so we could use different creds for the "true" repo vs the "pr" one, so that if creds ever leak from CI it would not affect released images.

mmlb commented 4 years ago

In the ongoing effort to release the pressure to our internal drone.io and to improve independency for Tinkerbell as a project, we should move our CI/CD to GitHub Actions.

tinkerbell/tink doesn't actually use our internal drone, tinkerbell/osie does. But this idea still makes sense since we want to do integration tests and part of that would be running workflows in VM based workers. We can only do that on self-hosted infra. So we'd either use our drone.packet.net (which we wouldn't want to make permanent) or a byo-server to GitHub Actions. I prefer the latter :+1:.

gianarb commented 4 years ago

Ok, thank you for explaining to me the difference between *-pr repo and not. I think it is cool and I will replicate that.

We decided to split into a separate image so we could use different creds for the "true" repo vs the "pr" one, so that if creds ever leak from CI it would not affect released images.

I can use something like QUAY_USERNAME_PR QUAY_PASSWORD_PR for PR and QUAY_USERNAME QUAY_PASSWORD for non PR, but not sure about how it will help with leaks because the workflow is actually the same

Another problem is that secrets gets not pushed to fork repo, so PR that comes from contributors that are not maintainers won't get the secrets required to push an image 😕

mmlb commented 4 years ago

Another problem is that secrets gets not pushed to fork repo, so PR that comes from contributors that are not maintainers won't get the secrets required to push an image confused

Bummer, but I'll take E2E tests over the -pr images any day.

gianarb commented 4 years ago

@displague this PR is not about e2e but it is more about porting what we have so far in drone. I will work on e2e later as soon as I will review and make #80 working