-
Notifications
You must be signed in to change notification settings - Fork 17
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
chore: Change repo config and curation to warning #141
chore: Change repo config and curation to warning #141
Conversation
@@ -1369,7 +1369,7 @@ fun RuleSet.noLicenseInDependencyRule() = dependencyRule("NO_LICENSE_IN_DEPENDEN | |||
|
|||
fun RuleSet.packageConfigurationInOrtYmlRule() = ortResultRule("PACKAGE_CONFIGURATION_IN_ORT_YML") { | |||
if (ortResult.repository.config.packageConfigurations.isNotEmpty()) { | |||
error( | |||
warning( | |||
"The use of package configurations is not allowed in the *.ort.yml file.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I'd propose to add a second commit to this PR which also changes the wording to say
The use of package configurations in the *.ort.yml file is currently disabled. Either enable the feature in the ORT configuration, or move package configurations to the $ortConfigVcsMdLink.
The current wording is overly strict IMO, and makes it sound as if the use of project-local package configurations would be discouraged or so. The same applies to the wording for package curations below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
Package configurations found in the *.ort.yml file. The recommended location for package configurations is $ortConfigVcsMdLink.
There is no way (that I know of) enable the feature in the ORT configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no way (that I know of) enable the feature in the ORT configuration.
There is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current wording is overly strict IMO, and makes it sound as if the use of project-local package configurations would be discouraged or so. The same applies to the wording for package curations below.
My intention when writing this rule was exactly that: A rule which discourages the use of package configurations in ort.yml
. Not wanting to discourage that is a valid case for sure, but in that case a user should simply not use this rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no way (that I know of) enable the feature in the ORT configuration.
There is.
I believe both properties, enableRepositoryPackageCurations
and enableRepositoryPackageConfigurations
are set to "true" with the below GitHub action, yet result is the same here:
https://github.com/scottschreckengaust/orter/actions/runs/6870755788/job/18686264762#step:3:1445
---
name: ort-toolkit
on:
push:
branches:
- main
workflow_dispatch: {}
jobs:
ort:
runs-on: ubuntu-latest
permissions:
contents: write
steps:
- name: Checkout project
uses: actions/checkout@v3
- name: Run GitHub Action for ORT
uses: oss-review-toolkit/ort-ci-github-action@7f23c1f8d169dad430e41df223d3b8409c7a156e
with:
ort-cli-args: '-P ort.forceOverwrite=true -P ort.enableRepositoryPackageConfigurations=true -P ort.enableRepositoryPackageCurations=true --stacktrace'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just agree that we disagree then.
I could live with reducing severity from error to warning as a resolution. Would that work for you as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean without also changing the wording of the messages? Yes, that would work as a compromise, until we're more clear about the practices to follow for this repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean without also changing the wording of the messages?
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, sorry @scottschreckengaust, would you then mind dropping your second commit again? Any while at it, probably change the first commit's title to use chore
(or refactor
) as a type rather than feat
. Thanks in advance!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed re-wording commit, and switched from feat
-> chore
in commit
Commit message:
This needs a different syntax as it's supposed to close an issue in a different repo: Also, the commit message body needs to be hard-wrapped at column 75. |
Because a repository configuration or curation may be purposeful, declare a `WARNING` instead of `ERROR` for the evaluator. Resolves oss-review-toolkit/ort#7753 Signed-off-by: Scott Schreckengaust <[email protected]>
A repository configuration or curation within an
.ort.yml
file may be purposeful. This declares aWARNING
instead ofERROR
for the evaluator.Resolves oss-review-toolkit/ort#7753
Please ensure that your pull request adheres to our contribution guidelines. Thank you!