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

Update code-path-changes.yml #3713

Merged
merged 3 commits into from
Feb 5, 2025
Merged

Update code-path-changes.yml #3713

merged 3 commits into from
Feb 5, 2025

Conversation

bretg
Copy link
Collaborator

@bretg bretg commented Jan 30, 2025

Turns out in order for this to work for forked repos the trigger needs to be 'pull_request_target' rather than 'pull_request'

🔧 Type of changes

  • new bid adapter
  • bid adapter update
  • new feature
  • new analytics adapter
  • new module
  • module update
  • bugfix
  • documentation
  • configuration
  • dependency update
  • tech debt (test coverage, refactorings, etc.)

✨ What's the context?

Code path notifications are not working for PRs from forked repos

Turns out in order for this to work for forked repos the trigger needs to be 'pull_request_target' rather than 'pull_request'
osulzhenko
osulzhenko previously approved these changes Jan 30, 2025
Copy link
Collaborator

@Net-burst Net-burst left a comment

Choose a reason for hiding this comment

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

This might be a potential security issue because this way all of the secrets are exposed. I think the common practice for when you need this is to limit the permissions. This should be enough for our use case:

permissions:
  contents: read

@bretg
Copy link
Collaborator Author

bretg commented Feb 4, 2025

Net-burst
Net-burst previously approved these changes Feb 4, 2025
Copy link
Collaborator

@Net-burst Net-burst left a comment

Choose a reason for hiding this comment

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

LGTM for now with the intention to see whether it will work in the first place and stricten the permissions later.

OZ convinced me that contents:read is better.
Copy link
Collaborator

@Net-burst Net-burst left a comment

Choose a reason for hiding this comment

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

LGTM

@CTMBNara CTMBNara merged commit 598c2c6 into master Feb 5, 2025
4 checks passed
@CTMBNara CTMBNara deleted the codepath-trigger-update branch February 5, 2025 16:14
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.

4 participants