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

replace labeler action with periodic-labeler #1097

Merged
merged 2 commits into from
May 8, 2020

Conversation

xchapter7x
Copy link
Contributor

this replaces the default labeler as it
does not label PRs from forks properly.

with periodic-labeler it should apply labels on
a cron to any PR and resolve the below bug:

#1092

this replaces the default labeler as it
does not label PRs from forks properly.

with periodic-labeler it should apply labels on
a cron to any PR and resolve the below bug:

spf13#1092
@xchapter7x
Copy link
Contributor Author

@jharshman @jpmcb

Thoughts on this PR as a fix for #1092 ?

Copy link
Collaborator

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jharshman
Copy link
Collaborator

Not sure what I think about this. It's a Github Action created by a 3rd party. Does this give anyone else pause?

@xchapter7x
Copy link
Contributor Author

fair point, @jharshman .

It is open source, so we can see exactly what it's doing here: https://github.com/paulfantom/periodic-labeler/blob/master/main.go
Doesn't seem to be leaking anything, just calling the Github API.

Also the container image used is distroless maintained by Google: https://github.com/GoogleContainerTools/distroless

We are pinning to a specific version of this Action, so it changing out from under us and compromising secrets somehow seems unlikely.

I guess we could fork it and maintain our own version of the Action, but that might be more burden than the risk is worth.

Your call :)

@jharshman
Copy link
Collaborator

We are pinning to a specific version of this Action, so it changing out from under us and compromising secrets somehow seems unlikely.

It's easy for a pinned version to get updated at some point in the future w/o undergoing another code review.

Can you add a comment in the action right above the image? It should indicate that if the version is to be updated, a code review of the 3rd party software should be performed to ensure that nothing malicious is going to be run.

@jharshman jharshman added the kind/bug A bug in cobra; unintended behavior label Apr 28, 2020
if we are to bump this version
then we should re-evaluate if there are
any leaks or exploits in the actions implementation

https://github.com/spf13/cobra/pull/1097\#issuecomment-620877596
@xchapter7x
Copy link
Contributor Author

We are pinning to a specific version of this Action, so it changing out from under us and compromising secrets somehow seems unlikely.

It's easy for a pinned version to get updated at some point in the future w/o undergoing another code review.

Can you add a comment in the action right above the image? It should indicate that if the version is to be updated, a code review of the 3rd party software should be performed to ensure that nothing malicious is going to be run.

latest commit adds the requested comment. @jharshman

Let me know if there are any other concerns we should address :)

@jharshman
Copy link
Collaborator

There's still a flaw in this that makes me want to fork it.
There is nothing stopping a person from overwriting a tag. Let me know if I'm being too paranoid about this.
@xchapter7x @jpmcb

@xchapter7x
Copy link
Contributor Author

There's still a flaw in this that makes me want to fork it.
There is nothing stopping a person from overwriting a tag. Let me know if I'm being too paranoid about this.
@xchapter7x @jpmcb

Agreed, the author of this action could retag and swap out the pinned version from under us.

The same, however, can be said for every go module or open source import.

Granted in go there is a go.sum to check if a modification happened somewhere in the chain, which guards against tag swapping. Which, as you point out, we're lacking here.

There is another aspect to this, in that its on the github marketplace. We can report wrong doing. We can see the code. We can subscribe and be notified when the code changes in the repo we're referencing. We can also see who the maintainers are, and in this case they seem to be associated with reputable companies and contributors to reputable OSS projects.

I see the risk as being rather low.

If tag swapping is a really high concern, we could consider pinning to a specific commit ref. That should work the same as pinning to a tag does, but i don't believe one can as easily swap that out from under us.

i'm guided by what you're comfortable with @jharshman :)

@jharshman
Copy link
Collaborator

@xchapter7x alrighty I think I'm good with this.
👍

@jharshman jharshman merged commit a7aaa7c into spf13:master May 8, 2020
@umarcor umarcor mentioned this pull request May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in cobra; unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants