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

Migrate to GitHub workflows #461

Merged
merged 56 commits into from
Jan 25, 2020

Conversation

alonyb
Copy link
Contributor

@alonyb alonyb commented Jan 7, 2020

This PR contains a new pipeline for this project with GitHub Workflows

There are 2 files, one for PUSH/PR and other for release.

PUSH & PR FILE:

  • Run all travis commands before deploying stage

RELEASE FILE:

  • Run all travis commands including deploying stage
  • For creating a new release, it must have a tag in the commit and then push the tag, and the pipeline will create a release automatically.

To see logs about the workflow see the fork on my repo here , both config files are included

This PR will help with this #451

@ahmetb
Copy link
Member

ahmetb commented Jan 13, 2020

@RubenBaez in the meanwhile, I've opened this issue suggesting we can move krew-index to GitHub Workflows. It should be much more straightforward since it's a smaller workflow.

Would you have cycles to do that? I think we can merge that much more comfortably.

@alonyb
Copy link
Contributor Author

alonyb commented Jan 14, 2020

@ahmetb
I can maintain it for a while and see that there are no problems :), and sure I will check it.

Copy link
Contributor

@corneliusweig corneliusweig left a comment

Choose a reason for hiding this comment

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

Hey @RubenBaez, this is looking great! Thanks a lot for your work.

The latest version with all-in-one file looks quite clean, I like it. Also maintenance-wise, this looks pretty reasonable.

One last concern that I have is: could we accidentally pick up a malicious action which steals the github token? I have no idea if and how the actions on the marketplace are screened. Is that a relevant concern, or am I just being paranoid?

I'm still curious what @ferhatelmas thinks about the latest version.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@corneliusweig
Copy link
Contributor

corneliusweig commented Jan 21, 2020

Ok, to me this looks quite good now.

There are twoone more things that we need to take care of:

@ahmetb
Copy link
Member

ahmetb commented Jan 21, 2020

We can remove Codecov altogether, I haven't been finding it super useful to be honest.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@alonyb
Copy link
Contributor Author

alonyb commented Jan 22, 2020

Ok, to me this looks quite good now.

There are twoone more things that we need to take care of:

@corneliusweig
As @ahmetb said

We can remove Codecov altogether, I haven't been finding it super useful, to be honest.

I didn't put it in the pipeline. Therefore. If you change your mind. Or then it becomes necessary, I could add that step :)

@alonyb alonyb requested a review from ahmetb January 22, 2020 03:48
@corneliusweig
Copy link
Contributor

Alright, let's put this on trial. As discussed, we'll run travis alongside for a while, see if the results are identical, and watch out for maintenance overhead.

Thanks for pushing this through! @alonyb

/lgtm
Since it's quite a delicate change, let's wait for a 👍 from @ahmetb.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2020
@ahmetb
Copy link
Member

ahmetb commented Jan 25, 2020

/approve
Thanks for the continuous work here. I hope this works out.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, alonyb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 25, 2020
@k8s-ci-robot k8s-ci-robot merged commit 5e5a4e7 into kubernetes-sigs:master Jan 25, 2020
@corneliusweig
Copy link
Contributor

Hey @alonyb,

I have one final request: You seem to follow closely how GH actions evolve. Should you learn that the env variable workaround (https://github.com/kubernetes-sigs/krew/pull/461/files#diff-e9f950f17198d3d5e3122a44230a09b9R32) can be done in a cleaner way (or can be skipped), could you open a follow-up PR for that?

@alonyb
Copy link
Contributor Author

alonyb commented Jan 29, 2020

@corneliusweig
Sure, I'm going to do some research and open the PR. No problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants