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 to GitHub action from Drone #235

Merged
merged 1 commit into from
Aug 6, 2020
Merged

Move to GitHub action from Drone #235

merged 1 commit into from
Aug 6, 2020

Conversation

gianarb
Copy link
Contributor

@gianarb gianarb commented Aug 5, 2020

Description

Move from Drone to Github Action

Why is this needed

Fixes: #233

How Has This Been Tested?

I am using act to run GitHub Action locally.

How are existing users impacted? What migration steps/scripts do we need?

none

  • go fmt
  • go vet
  • golang-ci
  • go test
  • It runs the ci-check script
  • build docker image tink-cli
  • build docker image tink-worker
  • build docker image tink-server

When merged to master:

  • push docker image tink-cli
  • push docker image tink-worker
  • push docker image tink-server

Copy link
Contributor

@mmlb mmlb left a comment

Choose a reason for hiding this comment

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

It would be nice to separate the lint fixes out of this PR and into a different one.

Comment on lines 28 to 29
- run: make
name: Build binaries
Copy link
Contributor

Choose a reason for hiding this comment

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

- name: Build binaries
  run: make

to match the order of all the other actions

repository: tinkerbell/tink-server
path: ./cmd/tink-server/
push: ${{ startsWith(github.ref, 'refs/heads/master') }}
tags: latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add

tag_with_ref: true
tag_with_sha: true

too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those options will push:

Successfully tagged quay.io/tinkerbell/tink:latest
Successfully tagged quay.io/tinkerbell/tink:pr-235-merge
Successfully tagged quay.io/tinkerbell/tink:sha-f009303

I don't think we want that

Copy link
Contributor

Choose a reason for hiding this comment

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

I do. Maybe not tag_wit_ref for pushes, but definitely for tags. But I do want the sha so we can have point-in-time refs to images, and not just latest that keeps getting pushed up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I will leave this tag_with_sha: true.
About tags, that's a separate discussion btw. It is not covered here and I will add it as soon as we get closer to a release lifecycle plan.

it does not look we are close to it

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree.

@gianarb gianarb requested review from mmlb and thebsdbox August 6, 2020 10:16
When a new PR gets open:

1. fmt
2. vet
3. test
4. golint-ci
5. ci-check with nixos
6. build docker images
7. push docker images to quay only for master

Signed-off-by: Gianluca Arbezzano <gianarb92@gmail.com>
@gianarb gianarb added the ready-to-merge Signal to Mergify to merge the PR. label Aug 6, 2020
Copy link
Contributor

@thebsdbox thebsdbox left a comment

Choose a reason for hiding this comment

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

/lgtm

@gianarb gianarb added ready-to-merge Signal to Mergify to merge the PR. and removed ready-to-merge Signal to Mergify to merge the PR. labels Aug 6, 2020
@gianarb gianarb merged commit 150d1d7 into tinkerbell:master Aug 6, 2020
@gianarb gianarb deleted the ci/gh branch August 6, 2020 15:02
@mmlb mmlb removed the ready-to-merge Signal to Mergify to merge the PR. label Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

From Drone to Github Action
3 participants