Skip to content

Conversation

@lucas-zimerman
Copy link
Contributor

@lucas-zimerman lucas-zimerman commented Oct 6, 2025

Based on V2.

The PR allows the danger CI to also include dangerfiles from the repo. This is good since only one github comment is allowed per repo, meaning that without this change, the danger used by the repo will overwrite the comment done by this repo.

This change adds a new step, loading an external dangerfile if set.
It passes the default parameters as exposing the following items:
fail

  • warn
  • message
  • markdown
  • danger

The changes were tested on the following PR:
getsentry/sentry-react-native#5235 (comment)

How external libraries should be implemented

Here is a example file using the changes: https://github.com/getsentry/sentry-react-native/blob/3dcd68b8db138e90aff72d51e46f89a6b0b70b0c/scripts/check-replay-stubs.js
If there is approval on merging these changes on this repo, I will update the readme file with guides on how to use an external dangerfile

#skip-changelog

@lucas-zimerman
Copy link
Contributor Author

@vaind What do you think of this feature?

@lucas-zimerman
Copy link
Contributor Author

Q: Should I target V3?

@vaind
Copy link
Collaborator

vaind commented Oct 8, 2025

@vaind What do you think of this feature?

Hey, haven't had a chance to look yet but makes sense in general.

@vaind
Copy link
Collaborator

vaind commented Oct 9, 2025

Note: danger-js has been updated by #132 (updater job added in #131)

@lucas-zimerman
Copy link
Contributor Author

sorry for the extra tests I added (they got reverted), I wasn't sure why danger for changelogs wasn't triggering, turned out there are new filters that it skips the changelog check based on the title description that didn't happen on V2.

@lucas-zimerman lucas-zimerman marked this pull request as ready for review October 9, 2025 12:59
@lucas-zimerman lucas-zimerman requested a review from vaind October 9, 2025 13:01
@lucas-zimerman lucas-zimerman marked this pull request as draft October 10, 2025 10:41
@lucas-zimerman
Copy link
Contributor Author

Testesd the following cases here:

Run with extra-dangerfile and extra-install-packages
https://github.com/getsentry/sentry-react-native/actions/runs/18411426970/job/52464497860

Run with extra-dangerfile and malicious shell script
https://github.com/getsentry/sentry-react-native/actions/runs/18411514303/job/52464814813?pr=5235

Run with no extra parameters
https://github.com/getsentry/sentry-react-native/actions/runs/18411569658/job/52465003681?pr=5235

Run with extra-dangerfile, extra-install-packages and a valid diff check for extra-dangerfile
https://github.com/getsentry/sentry-react-native/actions/runs/18412024717/job/52466642228?pr=5235

@lucas-zimerman lucas-zimerman marked this pull request as ready for review October 10, 2025 16:14
@lucas-zimerman
Copy link
Contributor Author

@vaind I added the following steps:

  • Validate package names Avoids running the CI if there is anything suspicious on extra-install-packages.
  • Setup container is the old Run DangerJS without the part of running danger.
  • Setup additional packages sets all new packages as root if set.
  • Run DangerJS like the original, doesn't run danger as root.

@vaind
Copy link
Collaborator

vaind commented Oct 14, 2025

@sentry review

lucas-zimerman and others added 6 commits October 14, 2025 16:39
Co-authored-by: Ivan Dlugos <6349682+vaind@users.noreply.github.com>
Co-authored-by: seer-by-sentry[bot] <157164994+seer-by-sentry[bot]@users.noreply.github.com>
Co-authored-by: seer-by-sentry[bot] <157164994+seer-by-sentry[bot]@users.noreply.github.com>
@lucas-zimerman
Copy link
Contributor Author

@sentry review

@lucas-zimerman lucas-zimerman requested a review from vaind October 14, 2025 16:32
--workdir /github/workspace \
--user $(id -u) \
-e "INPUT_ARGS" -e "GITHUB_JOB" -e "GITHUB_REF" -e "GITHUB_SHA" -e "GITHUB_REPOSITORY" -e "GITHUB_REPOSITORY_OWNER" -e "GITHUB_RUN_ID" -e "GITHUB_RUN_NUMBER" -e "GITHUB_RETENTION_DAYS" -e "GITHUB_RUN_ATTEMPT" -e "GITHUB_ACTOR" -e "GITHUB_TRIGGERING_ACTOR" -e "GITHUB_WORKFLOW" -e "GITHUB_HEAD_REF" -e "GITHUB_BASE_REF" -e "GITHUB_EVENT_NAME" -e "GITHUB_SERVER_URL" -e "GITHUB_API_URL" -e "GITHUB_GRAPHQL_URL" -e "GITHUB_REF_NAME" -e "GITHUB_REF_PROTECTED" -e "GITHUB_REF_TYPE" -e "GITHUB_WORKSPACE" -e "GITHUB_ACTION" -e "GITHUB_EVENT_PATH" -e "GITHUB_ACTION_REPOSITORY" -e "GITHUB_ACTION_REF" -e "GITHUB_PATH" -e "GITHUB_ENV" -e "GITHUB_STEP_SUMMARY" -e "RUNNER_OS" -e "RUNNER_ARCH" -e "RUNNER_NAME" -e "RUNNER_TOOL_CACHE" -e "RUNNER_TEMP" -e "RUNNER_WORKSPACE" -e "ACTIONS_RUNTIME_URL" -e "ACTIONS_RUNTIME_TOKEN" -e "ACTIONS_CACHE_URL" -e GITHUB_ACTIONS=true -e CI=true \
-e GITHUB_TOKEN="${{ inputs.api-token }}" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Security: Direct Input Interpolation

This line directly interpolates inputs.extra-dangerfile into the environment variable assignment. According to repository security patterns, user-controlled inputs should be assigned to job-level env variables first to prevent injection attacks.

Recommendation: Add a job-level env block and reference it as ${{ env.EXTRA_DANGERFILE }}.

Did we get this right? 👍 / 👎 to inform future reviews.

@lucas-zimerman
Copy link
Contributor Author

@vaind do you want me to apply the patches for the env suggestion? ( I don't see how it can make it safer but if you prefer I will apply it)

@vaind
Copy link
Collaborator

vaind commented Oct 16, 2025

@lucas-zimerman yes please. script inputs should always be coming through the env var to prevent injection

 fix reference

use instefof instead of invalid contains

use
@lucas-zimerman
Copy link
Contributor Author

@vaind fixed the env var and also some nits

Copy link
Collaborator

@vaind vaind left a comment

Choose a reason for hiding this comment

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

Minor changes but should be good to go. I assume you've tested this in your repo already?

@lucas-zimerman
Copy link
Contributor Author

Minor changes but should be good to go. I assume you've tested this in your repo already?

Correct All the changes I have been testing here:

getsentry/sentry-react-native#5235

image

Dange Action: https://github.com/getsentry/sentry-react-native/actions/runs/18656204660/job/53185832158?pr=5235

@lucas-zimerman
Copy link
Contributor Author

vaind and others added 4 commits October 29, 2025 09:22
…install-packages features

Add test coverage for the new extra-dangerfile and extra-install-packages inputs:

- Create test-dangerfile.js demonstrating custom Danger checks
- Add extra-dangerfile-test job to verify custom dangerfiles execute correctly
- Add extra-packages-test job to verify package installation works
- Tests validate that custom dangerfiles can access the Danger API and installed packages

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@vaind
Copy link
Collaborator

vaind commented Oct 29, 2025

@lucas-zimerman I've made some minor adjustments, added integration tests and merged the changes. If I've accidentally broken something with your code, we can followup in another PR.

@vaind vaind merged commit d6ae7f7 into getsentry:main Oct 29, 2025
5 checks passed
@vaind
Copy link
Collaborator

vaind commented Oct 29, 2025

#skip-changelog

I didn't notice this in the PR description - it's a feature and should have had a changelog

vaind added a commit that referenced this pull request Oct 29, 2025
@lucas-zimerman
Copy link
Contributor Author

Thank you for all the reviews! the changers are working great when I point to the latest commit hash from this repo.

@lucas-zimerman lucas-zimerman deleted the lz/ext-danger branch October 29, 2025 11:05
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