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

*: convert to single JS action #582

Merged
merged 1 commit into from
May 28, 2024
Merged

*: convert to single JS action #582

merged 1 commit into from
May 28, 2024

Conversation

thypon
Copy link
Member

@thypon thypon commented May 8, 2024

No description provided.

@thypon thypon requested a review from a team as a code owner May 8, 2024 10:10
@thypon thypon changed the title action.yml: convert basic shell-bash action.yml: convert shell-bash May 8, 2024
@thypon thypon marked this pull request as draft May 8, 2024 12:58
@thypon thypon changed the title action.yml: convert shell-bash action.yml: move to a single JS action May 8, 2024
@thypon thypon force-pushed the features/convert-to-js branch 17 times, most recently from 146dbe8 to 712d5c4 Compare May 15, 2024 13:26
@thypon thypon force-pushed the features/convert-to-js branch 6 times, most recently from c837314 to 7b89798 Compare May 17, 2024 10:27
@thypon thypon requested a review from diracdeltas May 24, 2024 15:37
@thypon thypon marked this pull request as ready for review May 24, 2024 15:37
@thypon thypon force-pushed the features/convert-to-js branch 5 times, most recently from 2a1ac21 to 02e3c34 Compare May 24, 2024 23:04
@brave brave deleted a comment from github-actions bot May 24, 2024
@thypon thypon changed the title [draft] action.yml: move to a single JS action *: convert to single JS action May 24, 2024
@thypon thypon changed the title *: convert to single JS action [draft] *: convert to single JS action May 24, 2024
@thypon thypon changed the title [draft] *: convert to single JS action *: convert to single JS action May 24, 2024
@@ -19,7 +19,7 @@ jobs:
python3 -m pip --disable-pip-version-check install -r requirements.txt
shell: bash
- run: |
semgrep --test --disable-version-check --strict --metrics=off
cd assets/semgrep_rules/; semgrep --test --disable-version-check --strict --metrics=off
Copy link
Member

Choose a reason for hiding this comment

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

nit: whitespace at end of line

action.cjs Outdated Show resolved Hide resolved
action.cjs Outdated Show resolved Hide resolved
Copy link
Member

@diracdeltas diracdeltas left a comment

Choose a reason for hiding this comment

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

nice refactor!

@thypon thypon force-pushed the features/convert-to-js branch 5 times, most recently from e59cb39 to 80bf9a5 Compare May 28, 2024 17:37
Copy link

[puLL-Merge] - brave/security-action@582

Description

This PR makes several changes to the security-action GitHub Action to improve its functionality and maintainability. The main motivation for these changes seems to be refactoring the action to use a single JavaScript file (action.cjs) to handle the main logic, rather than using multiple steps in the action.yml file.

Changes

Changes

  • .github/workflows/full-loop.yml: Adds set -e to fail the step if any command fails
  • .github/workflows/loop.yml: Adds slack_token input
  • .github/workflows/semgrep-self-test.yml: Changes directory to assets/semgrep_rules/ before running semgrep --test
  • action.cjs (new): Main JavaScript file that handles the core logic of the action. Imports functions from files in src/ to handle different steps.
  • action.yml:
    • Refactors to use a single JavaScript file (action.cjs) via actions/github-script rather than multiple steps
    • Simplifies logic for determining when to run reviewdog
    • Removes many individual steps that are now handled in action.cjs
  • assets/semgrep_rules/client/licensing7.txt (new): Adds a new Semgrep rule
  • package.json: Adds markdown-to-txt dependency
  • src/pullRequestChangedFiles.js (new): Function to get changed files in a PR
  • src/sendSlackMessage.js:
    • Adds support for text parameter in addition to message
    • Adds support for color parameter to set Slack message color
    • Adds logic to find Slack channel ID from name
    • Adds debouncing of duplicate messages in last 24 hours
  • src/steps/assigneeRemoved.js, src/steps/assigneesAfter.js, src/steps/hotwords.js: Minor refactoring

Possible Issues

The debouncing logic in src/sendSlackMessage.js could potentially prevent legitimate duplicate messages from being sent if an issue occurs multiple times in 24 hours. It may be worth adding a way to bypass the debounce in certain cases.

Overall this is a significant refactoring of the action but the changes look reasonable. I would suggest thorough testing, especially around the Slack integration and debouncing, to ensure no unexpected behavior. The maintainability improvements from consolidating the logic into action.cjs are a good change.

@thypon thypon merged commit 91a69f4 into main May 28, 2024
8 checks passed
@thypon thypon deleted the features/convert-to-js branch May 28, 2024 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants