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.

Sign up for GitHub

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

Jump to bottom

Remove SUCCESS comments for phylum-ci #78

Closed
3 tasks
maxrake opened this issue Jul 13, 2022 · 2 comments · Fixed by #526
Closed
3 tasks

Remove SUCCESS comments for phylum-ci #78

maxrake opened this issue Jul 13, 2022 · 2 comments · Fixed by #526
Assignees
@maxrake
Labels
enhancement New feature or request needs triage Used to indicate that an issue hasn't been reviewed

Comments

@maxrake
Copy link
Contributor

maxrake commented Jul 13, 2022 •
edited
Loading

Overview

Is your feature request related to a problem? Please describe.
Feedback from initial use of the GitHub/GitLab integrations has shown that the SUCCESS comments that get added to a PR/MR are considered too "noisy," specifically for the case where changes to the lockfile did not introduce any failing dependency checks across the risk domains.

Describe the solution you'd like
Eliminate the SUCCESS comment entirely

Describe alternatives you've considered

  • Don't get rid of the SUCCESS comment but do use it more sparingly
    • Only for those situations where there was a non-SUCCESS comment posted first:
      • FAILED
      • INCOMPLETE
      • INCOMPLETE WITH FAILURE
  • Re-write a single Phylum comment, in place, each time the PR changes

Additional context

The current integration behavior is to write a SUCCESS comment when the lockfile is analyzed and passes the thresholds. The thinking behind that design choice was to make it more obvious that the PR/MR included changes to the lockfile and that they were analyzed.

The lockfile will be analyzed when it has changed or when the --force-analysis flag is specified. If the lockfile is not analyzed (b/c it didn't change or --force-analysis wasn't provided), then NO PR comment will be written. The status check will show as "green" there, though.

Acceptance criteria

  • SUCCESS comments are no longer posted
  • Phylum comments on any MR/PR are only posted when there is something negative to say
  • Documentation is updated
@maxrake maxrake added enhancement New feature or request needs triage Used to indicate that an issue hasn't been reviewed labels Jul 13, 2022
@maxrake
Copy link
Contributor Author

maxrake commented Apr 10, 2023

The Phylum GitHub App already implements the behavior described in this issue. That might elevate this one so that the Action and App have matching behavior.

@maxrake maxrake self-assigned this Jan 13, 2025
@maxrake
Copy link
Contributor Author

maxrake commented Jan 13, 2025

After further discussion with a current customer and @furi0us333, it was decided to go with one of the alternative solutions:

Eliminate the SUCCESS comment entirely

This issue will be updated to reflect the new guidance.

@maxrake maxrake changed the title Remove initial SUCCESS comments for phylum-ci Remove SUCCESS comments for phylum-ci Jan 13, 2025
maxrake added a commit that referenced this issue Jan 13, 2025
@maxrake
feat!: remove SUCCESS comments from PRs/MRs
0cb7e72 
This change removes the `SUCCESS` comments from pull requests (PRs) and
merge requests (MRs). Other comment types are unchanged. This change is
necessary because the `SUCCESS` comments are considered by existing
customers to be too noisy and not useful since the related status check
will show as passing when the analyis is successful. Plus, the logs
contain the same information as was included in the `SUCCESS` comments.

Closes #78

BREAKING CHANGE: PRs/MRs will no longer have `SUCCESS` comments.
@maxrake maxrake mentioned this issue Jan 13, 2025
Merged
5 tasks
maxrake added a commit that referenced this issue Jan 14, 2025
@maxrake
feat!: remove SUCCESS comments from PRs/MRs (#526)
6cca5bd 
This change removes the `SUCCESS` comments from pull requests (PRs) and
merge requests (MRs). Other comment types are unchanged. This change is
necessary because the `SUCCESS` comments are considered by existing
customers to be too noisy and not useful since the related status check
will show as passing when the analysis is successful. Plus, the logs
contain the same information as was included in the `SUCCESS` comments.

Closes #78

BREAKING CHANGE: PRs/MRs will no longer have `SUCCESS` comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@maxrake maxrake

Labels
enhancement New feature or request needs triage Used to indicate that an issue hasn't been reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat!: remove SUCCESS comments from PRs/MRs phylum-dev/phylum-ci
1 participant
@maxrake