-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: undeploy-my-kibana #140
Conversation
the github label should be the gatekeeper
…tory.defaultBranchRef)
github-token: | ||
description: 'The GitHub Personal Access Token.' | ||
required: false | ||
github-app-id: | ||
description: 'The GitHub App ID to generate the ephemeral token.' | ||
required: false | ||
github-app-private-key: | ||
description: 'The GitHub App Private Key to generate the ephemeral token.' | ||
required: false |
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 don't know whether keeping both approaches might be good... WDYT?
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 think the GitHub app credentials are only a temporary solution. I understand the benefit right now, but I don't think we want GitHub app credentials as GitHub secrets in the future, as they are very powerful.
Also, as we know, a new approach is on the horizon already.
Hence, we should not add it here and just pass the GH token from the output of the tibdex action.
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 think it's even more protected this implementation since it pretty much generates the token with the expected permissions.
While if using the other approach implies the consumers will need to know what kind of permissions need be generated.
I somehow feel this approach, even if temporary, will facilitate the consumers since they don't need to know much. WDYT?
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.
Good point.
On the other hand, there is only one consumer. https:/search?q=org%3Aelastic+elastic%2Fapm-pipeline-library%2F.github%2Factions%2Fundeploy-my-kibana+%28path%3A**%2F*.yml+OR+path%3A**%2F*.yaml%29&type=code
And it's basically us maintaining it. (this may be true also for other oblt-actions, but I think this one is even more true for this one)
I'm also torn now.
The only thing I can think of is that we don't have to remove the inputs in a breaking change in the future if we don't add them in the first place.
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 agree about the breaking change
. I initially didn't implement see dcb19ad, but to a certain extent, I found it a bit complicated when I wrote the tests, see d0db167 - it's much cleaner.
In any case, we can support both cases and then once we have the new GH ephemeral token, the change will be pretty much using the github-token
input, so we won't need to care about the other two input parameters, we just won't use them
|
||
### Further details | ||
|
||
Caused by @${{ env.PR_AUTHOR }} in https:/${{ env.REPO }}/pull/${{ env.PR }} |
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 think I might also include the workflow URL that triggered this action
…ibana * upstream/main: (51 commits) deps: Bump oblt-cli version to 7.6.2 (elastic#139) feat: undeploy-my-kibana (elastic#140) build(deps): bump the github-actions group across 2 directories with 2 updates (elastic#141) build(deps): bump the github-actions group across 6 directories with 1 update (elastic#138) chore: deps(oblt-cli): Bump oblt-cli version to 7.5.24 (elastic#137) feat: support wait for maven central (elastic#133) feat: migrate is-member-elastic-org (elastic#135) deps: Bump oblt-cli version to 7.5.22 (elastic#131) deps(updatecli): bump all policies (elastic#130) ci: use GitHub app for ephemeral tokens (elastic#129) Deprecate the `project-id` input in `google/auth` action. (elastic#124) deps(updatecli): bump all policies (elastic#122) chore: deps(oblt-cli): Bump oblt-cli version to 7.5.21 (elastic#121) build(deps): bump the github-actions group across 11 directories with 4 updates (elastic#125) github-action: use ephemeral tokens with the required permissions (elastic#113) feat(github): validate-comment (elastic#120) feat(pre-commit): migrate from apm-pipeline-library (elastic#119) deps(updatecli): bump all policies (elastic#117) feat(await-maven-artifact): migrate from https:/elastic/apm-pipeline-library (elastic#118) Add `test-report` action (elastic#114) ...
Interestingly, e2e tests work but production workflow doesn't work:
I cannot understand what's the different between these two:
unless the checkout step is somehow related ?! |
and it works as expected now https:/elastic/kibana/actions/runs/11231085849/job/31219753582?pr=190211 |
What
An initial attempt to move away from a finer-grained token and use ephemeral tokens.
Further details
It's difficult to migrate https:/elastic/apm-pipeline-library/tree/main/.github/actions/undeploy-my-kibana to use the GH App ephemeral tokens.
It requires different permissions to access the members of a given org, so it can add an extra validation if the PR author is a member of Elastic.
I decided to remove that because it will require further complicated code, and the GitHub labels filtering in the workflows are good enough: