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

fix: webhook expects REPOSITORY_ALLOW_LIST env var #3856

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

galargh
Copy link
Contributor

@galargh galargh commented Apr 20, 2024

Description

The ConfigResolver of the webhook lambda expects the env variable to be named REPOSITORY_ALLOW_LIST (see

const repositoryAllowListEnv = process.env.REPOSITORY_ALLOW_LIST ?? '[]';
) whereas the one set by the webhook module was REPOSITORY_WHITE_LIST. I think it'd be great to follow up on #992, but it is beyond the scope of the fix proposed here now.

@npalm
Copy link
Member

npalm commented Apr 22, 2024

Thanks for creating the PR. This problem was noticed already in the PR #3830. As mentioned in that PR it is great to fix this.

But we are planning the depcreate this feture as well. The feature is from the early days of the module before runner groups were existing. Wondering what the use case is for this allow list.

@galargh
Copy link
Contributor Author

galargh commented Apr 22, 2024

Oh, I see. Sorry about that, I somehow missed this in a search. To be fair, I only noticed it when working on an unrelated feature (#3855) and quickly put up the fix PR.

I totally understand the plans to deprecate the feature altogether. One use case I did have was to enable a 2-step verification process of allowing new repositories to access the self-hosted runners. As in, it is not enough to allow webhook delivery to a new repository in the GitHub App, but you also have to explicitly modify the infrastructure (i.e. modify the allow list).

Thank you for the explanation and the link 🙇

@npalm
Copy link
Member

npalm commented Apr 22, 2024

Oh, I see. Sorry about that, I somehow missed this in a search. To be fair, I only noticed it when working on an unrelated feature (#3855) and quickly put up the fix PR.

I totally understand the plans to deprecate the feature altogether. One use case I did have was to enable a 2-step verification process of allowing new repositories to access the self-hosted runners. As in, it is not enough to allow webhook delivery to a new repository in the GitHub App, but you also have to explicitly modify the infrastructure (i.e. modify the allow list).

Thank you for the explanation and the link 🙇

Thanks for the feedback. In case we have the option to check the event is part of the group for which it is scaling the runners. Would that be good enough. Implementing this is not straighforward due to a lack of GitHub api's.

@npalm npalm merged commit 0006ab9 into philips-labs:main Apr 22, 2024
41 checks passed
npalm pushed a commit that referenced this pull request Apr 24, 2024
🤖 I have created a release *beep* *boop*
---


##
[5.10.1](v5.10.0...v5.10.1)
(2024-04-24)


### Bug Fixes

* Add missing webhook_events_workflow_job_queue_policy to multi-runner
queue
([#3848](#3848))
([a8cba4e](a8cba4e))
* **lambda:** bump the aws group in /lambdas with 5 updates
([#3861](#3861))
([6119354](6119354))
* **lambda:** bump typescript from 5.3.3 to 5.4.5 in /lambdas
([#3863](#3863))
([e3f3d77](e3f3d77))
* webhook expects REPOSITORY_ALLOW_LIST env var
([#3856](#3856))
([0006ab9](0006ab9))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: forest-releaser[bot] <80285352+forest-releaser[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants