Skip to content

Conversation

@booxter
Copy link
Contributor

@booxter booxter commented Apr 15, 2025

Otherwise the tested fork with the PR on review is not validated in CI.

Signed-off-by: Ihar Hrachyshka [email protected]

Otherwise the tested fork with the PR on review is not validated in CI.

Signed-off-by: Ihar Hrachyshka <[email protected]>
@mergify mergify bot added the CI/CD Affects CI/CD configuration label Apr 15, 2025
@booxter
Copy link
Contributor Author

booxter commented Apr 15, 2025

@courtneypacheco @ktdreyer please confirm this is safe / correct.

@ktdreyer
Copy link
Contributor

@ktdreyer
Copy link
Contributor

I did more reading here: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/

Now I have learned more about the differences here:

pull_request will run on the PR's code, and it will not have access to any the secrets in our repositories (so it won't be able to run AWS tests).

pull_request_target does have access to secrets, but we should not use it to run untrusted PR code.

@booxter
Copy link
Contributor Author

booxter commented Apr 16, 2025

@ktdreyer since I switched unit tests to use github runners, is it safe to switch unit tests job to pull_request? Will it give us proper validation for PRs under review? I've checked what we do in CLI repo and we use pull_request there too, so I assume it's safe?

Copy link
Contributor

@ktdreyer ktdreyer left a comment

Choose a reason for hiding this comment

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

Yes, thanks for clarifying.

I see #475 cleans up the ec2 reference, and I understand this only uses GH's own runners now.

Copy link
Contributor

@JamesKunstle JamesKunstle left a comment

Choose a reason for hiding this comment

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

I was using 'pull_request_target' because I thought that was required for the EC2 runners, but since we're no longer using those runners for unit testing this totally makes sense to me.

@mergify mergify bot removed the one-approval label Apr 16, 2025
@JamesKunstle JamesKunstle merged commit d34fc40 into instructlab:main Apr 16, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI/CD Affects CI/CD configuration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants