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

Improve GitHub Actions hazelcast membership check #1530

Conversation

JackPGreen
Copy link
Contributor

We restrict actions' execution to members of the hazelcast organisation using hazelcast/hazelcast-tpm/membership, passing in the user derviced from github.event.pull_request.head.repo.owner.login.

This is the owner of the repository, not the user itself.

In the case of a non-fork PR - i.e. one in the hazelcast original repo - the owner will be hazelcast (which isn't a GitHub user, and isn't a member of the hazelcast organisation) and will fail.

This nuance prevented hazelcast/hazelcast-python-client#720 from being merged.

Instead we should query github.actor as already used in the C++ client.

We restrict actions' execution to members of the `hazelcast` organisation using `hazelcast/hazelcast-tpm/membership`, passing in the user derviced from `github.event.pull_request.head.repo.owner.login`.

This is the owner of the repository, _not_ the user itself.

In the case of a non-fork PR - i.e. one in the `hazelcast` original repo - the `owner` will be `hazelcast` (which _isn't_ a GitHub user, and _isn't_ a member of the `hazelcast` organisation) and will fail.

This nuance prevented hazelcast/hazelcast-python-client#720 from being merged.

Instead we should query `github.actor` as [already used in the C++ client](https://github.com/hazelcast/hazelcast-cpp-client/blame/b63e40748aa8e06f790510d86e1eae87cc36925a/.github/workflows/build-pr.yml#L50).
@JackPGreen JackPGreen enabled auto-merge (squash) February 18, 2025 23:17
@JackPGreen
Copy link
Contributor Author

@ihsandemir are you able to review, please?

@JackPGreen
Copy link
Contributor Author

@ihsandemir are you able to review, please?

bump :)

@JackPGreen
Copy link
Contributor Author

@ihsandemir are you able to review, please?

bump :)

@ihsandemir just a reminder on this one :)

Copy link

@ihsandemir ihsandemir left a comment

Choose a reason for hiding this comment

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

Is there a sample test run you can share which shows that it works in this repo?

@JackPGreen
Copy link
Contributor Author

JackPGreen commented Mar 24, 2025

Is there a sample test run you can share which shows that it works in this repo?

https://github.com/hazelcast/hazelcast-nodejs-client/actions/runs/14047006191/job/39329912784

EDIT - The action overall fails, but the membership step still passes (the bit I've changed). I'm not planning on fixing anything else (at least, not now).

@JackPGreen JackPGreen disabled auto-merge March 25, 2025 07:46
@JackPGreen JackPGreen merged commit b55e48f into hazelcast:master Mar 25, 2025
7 of 13 checks passed
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