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

From Drone to Github Action #233

Closed
gianarb opened this issue Aug 5, 2020 · 9 comments · Fixed by #235
Closed

From Drone to Github Action #233

gianarb opened this issue Aug 5, 2020 · 9 comments · Fixed by #235

Comments

@gianarb
Copy link
Contributor

gianarb commented Aug 5, 2020

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, Introduce golangci-lint for CI tests #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
Copy link
Contributor

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
Copy link
Contributor Author

gianarb commented Aug 5, 2020

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
Copy link
Member

displague commented Aug 5, 2020

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
Copy link
Contributor

thebsdbox commented Aug 5, 2020

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
Copy link
Contributor

mmlb commented Aug 5, 2020

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
Copy link
Contributor

mmlb commented Aug 5, 2020

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 👍.

@gianarb
Copy link
Contributor Author

gianarb commented Aug 5, 2020

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
Copy link
Contributor

mmlb commented Aug 5, 2020

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
Copy link
Contributor Author

gianarb commented Aug 6, 2020

@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

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

Successfully merging a pull request may close this issue.

4 participants