Skip to content

feat: add dirty-waters workflow to CI#6182

Open
randomicecube wants to merge 20 commits into
INRIA:masterfrom
randomicecube:dirty-waters
Open

feat: add dirty-waters workflow to CI#6182
randomicecube wants to merge 20 commits into
INRIA:masterfrom
randomicecube:dirty-waters

Conversation

@randomicecube
Copy link
Copy Markdown

@randomicecube randomicecube commented Feb 17, 2025

cc @monperrus @Stamp9

Relates to #5216, chains-project/dirty-waters#37, chains-project/dirty-waters#58

Key notes:

  • x_to_fail parameter: the percentage of a single non-high severity issue present among the dependencies for the CI to break. It defaults to 5%, so what should it be here?
  • comment_on_commit: whether the reports are allowed to be pasted as comments in the commits, in the case of high-severity issues breaking CI. Defaults to false, what do we want here?
  • allow_pr_comments: whether the reports are allowed to be pasted as comments in pull requests. Defaults to true, what do we want here?
  • By default, only static analysis happens. Do we want differential to be performed in certain scenarios too? If so, which ones?
  • Gradual reporting is enabled by default -- only one of the reported smells has its table displayed per time, with the higher severity issues showing up first.
  • In chore: add dirty waters chains-project/sbom.exe#317, @algomaster99 included a step before the analysis which builds the project and skips tests; should the same be done here?

For more information on the action, see the wiki.

@algomaster99
Copy link
Copy Markdown
Contributor

included a step before the analysis which builds the project and skips tests; should the same be done here?

Skipping tests first time is necessary for me because I want to build the agent to run the tests. But if run the tests straightaway, the agent does not exist so tests fail and I also don't get the built agent.
But I would recommend doing so. I don't think so any of the software supply chain smells in dirty-waters are test dependent.

@monperrus
Copy link
Copy Markdown
Collaborator

x_to_fail parameter: the percentage of a single non-high severity issue present among the dependencies for the CI to break. It defaults to 5%, so what should it be here?

the current value, we should never go higher than this.

comment_on_commit: whether the reports are allowed to be pasted as comments in the commits, in the case of high-severity issues breaking CI. Defaults to false, what do we want here?

false

allow_pr_comments: whether the reports are allowed to be pasted as comments in pull requests. Defaults to true, what do we want here?

If the comment it only in case of CI failure yes, else no.

By default, only static analysis happens. Do we want differential to be performed in certain scenarios too? If so, which ones?

no, not needed for this MVP

Gradual reporting is enabled by default -- only one of the reported smells has its table displayed per time, with the higher severity issues showing up first.

full reporting.

thanks @randomicecube

@randomicecube
Copy link
Copy Markdown
Author

If the comment it only in case of CI failure yes, else no.

It currently isn't, but I'll make it so

Thanks for the feedback, I'll ping again when everything is done

@I-Al-Istannen
Copy link
Copy Markdown
Collaborator

I also want to note that I will not merge any per-commit/PR workflow that takes longer than ~10 Minutes (ideally much shorter). The CI already takes ages to run, I don't want to wait 40 minutes for a result.

@randomicecube
Copy link
Copy Markdown
Author

@I-Al-Istannen Absolutely, there's a bug which I'm working on fixing right now which is making it so that the cache isn't being stored with the correct key -- I'll fix it, test it, link the test results (and run them here too)
With caching, the action is fast!

@randomicecube
Copy link
Copy Markdown
Author

randomicecube commented Feb 19, 2025

From my tests in the fork I have, this doesn't work yet either with v1.8, so no need to accept the workflow just yet

EDIT: Ok you were right, it's due to caching inheritance! Because, for example, in this commit in a main branch, the cache is correctly stored; furthermore, in this PR, the action is triggered both by push and by PR: the push one stores correctly, the PR one doesn't!

@monperrus
Copy link
Copy Markdown
Collaborator

@randomicecube can we get that one merged?

@randomicecube
Copy link
Copy Markdown
Author

@monperrus haven't touched on the project in a bit but Maven should work fine (bear in mind, as previously discussed, cold runs may take a while, subsequent ones should be fast)

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