-
Notifications
You must be signed in to change notification settings - Fork 291
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
Upload plugins to GCS #944
Conversation
9b249c9
to
9f30346
Compare
Hey @josefkarasek, please post the links to sample runs of this Job on your fork (as a proof this works as expected). Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM, good work! 👍 Just minor comments.
Also, I know we're also building these plugins for integration test purposes, so theoretically we could extend that workflow, but building plugins is fast and I think a separate workflow like you did is much easier to maintain 👍
publish_plugins: | ||
permissions: | ||
contents: 'read' | ||
id-token: 'write' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? 🤔
version: latest | ||
- name: Build all plugins into dist directory | ||
env: | ||
GORELEASER_CURRENT_TAG: "v1.0.0-devel" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I know we used this for integration tests, but this is different case, because the version is public. Can we change this v9.99.9-dev
? It's not that I like v9.99.9-dev
tag, but just for consistency reason it would be better than 1.0.0. Somebody can think we're going to release 1.0.0 soon 😄
Other ideas: v0.0.0-latest
, v0.0.0-dev
... but probably consistency is better.
What do you think?
PS you can also change the version for the integration tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I there any place in the test where the version is also hard-coded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, branch + PR build, but you already changed 👍
d5791aa
to
3196c52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before merge, can you run this job once again just to make sure it works properly? I think the content: read
permission is necessary (I wanted to remove id_token
: write). But let's make sure it works before merge 👍 You can post a link to successful job here. Thanks!
3196c52
to
2fdfd82
Compare
For example this run: https://github.com/kubeshop/botkube/actions/runs/3949008561/workflow |
f2fedb1
to
9db87dc
Compare
Description
Changes proposed in this pull request:
Resolves #943