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

Use imjasonh/setup-ko GitHub Action #747

Merged
merged 1 commit into from
Apr 30, 2021

Conversation

imjasonh
Copy link
Contributor

See https://github.com/imjasonh/setup-ko

This action installs the latest ko release, so we don't need to bump
the version ourselves every time there's a release.

We can also pin to a ko release if we want to, to avoid a breakage or
bad version.

/kind cleanup

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

NONE

@openshift-ci-robot openshift-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-none Label for when a PR does not need a release note labels Apr 29, 2021
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2021
@qu1queee qu1queee self-requested a review April 29, 2021 05:35
Copy link
Contributor

@qu1queee qu1queee left a comment

Choose a reason for hiding this comment

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

cool stuff
/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qu1queee

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 29, 2021
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

@imjasonh from a security pov, and I am fully trusting you, but, I think that actions preferable should be in some common org where some review is happening to ensure that secrets are not leaked eventually. Current action imports are from actions, azure, helm and docker which is I think where somebody has an eye on.

I am not fully knowing the versioning schema when consuming these actions, but I think v0.3 would allow you to make updates. The document suggests to reference by sha to make sure that code is not getting changed.

Maybe worth a quick discussion on how we want to consume actions from personal accounts. Imo we need some rules there. Holding PR for now.

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 29, 2021
@imjasonh
Copy link
Contributor Author

@SaschaSchwarze0 That's an entirely reasonable concern.

I'm fine with pinning it to a specific commit, if that's what you'd prefer. I'm also fine with copying the action into shipwright-io if we think that's appropriate, so any changes get reviewed; that feels a bit like overkill to me, but I'd be willing to consider it.

However we pin or version the action definition, it'll still install the floating latest ko release, unless we tell it not to. If that's not what we want then there's little benefit to using the action at all in our case.

@SaschaSchwarze0
Copy link
Member

@SaschaSchwarze0 That's an entirely reasonable concern.

I'm fine with pinning it to a specific commit, if that's what you'd prefer. I'm also fine with copying the action into shipwright-io if we think that's appropriate, so any changes get reviewed; that feels a bit like overkill to me, but I'd be willing to consider it.

However we pin or version the action definition, it'll still install the floating latest ko release, unless we tell it not to. If that's not what we want then there's little benefit to using the action at all in our case.

In my opinion, referencing the action version by SHA is what we need to do. I am okay that the action installs the latest ko release because ko is backed by a community.

I do not think it makes sense to copy the action into the shipwright organization, it does not really belong there. From my perspective it would rather make more sense if you contribute it to the ko community.

And just to make sure I am not misunderstood: I was holding this PR not because of lacking trust in you. Instead, I think that referencing third-party dependencies (programming libraries, base images, or github actions) generally is becoming a more and more relevant threat in our business. Be it that dependencies disappear from one day to the next one (like left-pad) or that repositories get hacked and malicious code injected (like event-stream.

@imjasonh
Copy link
Contributor Author

From my perspective it would rather make more sense if you contribute it to the ko community.

I'd prefer to do that too (I originally proposed it in ko-build/ko#347, removed in ko-build/ko#352), but Google apparently hasn't accepted GitHub's Marketplace Terms of Service so you can't publish actions from the github.com/google org. If "the ko community" existed outside of the google org it would be a possibility, and a high likelihood. But that's a larger topic.

And just to make sure I am not misunderstood: I was holding this PR not because of lacking trust in you.

Oh don't worry, there's no misunderstanding, and no offense interpreted on my part. It's good and healthy to be skeptical of these sorts of dependencies. There's also the Bus Factor to consider 😆 🚌

I'll update the PR to reference by SHA, and if anybody else has any comments/concerns we can address them.

Thanks for your feedback, and for wanting to do this securely and reliably. 👍

See https://github.com/imjasonh/setup-ko

This action installs the latest `ko` release, so we don't need to bump
the version ourselves every time there's a release.

We can also pin to a ko release if we want to, to avoid a breakage or
bad version.

Referencing the action by SHA (equivalent to v0.3) to avoid silent
behavior changes and breakages.
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

/lgtm
/unhold

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 30, 2021
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2021
@openshift-merge-robot openshift-merge-robot merged commit a77aa59 into shipwright-io:master Apr 30, 2021
@qu1queee
Copy link
Contributor

Is this discussion something we should move to the community meeting?, mainly because of the following aspects:

  • what makes a source trustable? there are github orgs that are composed of very few people, so not because source belongs to an org it means is more trustable. Take as an example https://github.com/homeport which is maintained mainly by @HeavyWombat . I would like to avoid situations in the future were we denied ourselves of quality code because of this concerns. I also would like to be flexible on embracing independent actions in shipwright, as long as they are compliant with particular protocols ( something we can define ourselves )
  • I think the security concerns are relevant, as for example using a SHA instead of a tag. However, there are layers like the backend logic that makes the actions workable in github that are also vulnerable ( see example ), but that doesnt mean we stop using github actions.

@imjasonh
Copy link
Contributor Author

Sure, I've added it to the agenda. If we decide it doesn't make sense to depend on this, I'm fine reverting this change.

@adambkaplan adambkaplan added this to the release-v0.5.0 milestone Jun 10, 2021
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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. release-note-none Label for when a PR does not need a release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants