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

feat(jenkins): Add a parameter to skip excluded scopes and paths #8219

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

sschuberth
Copy link
Member

Enable this by default for performance reasons of showcasing ORT. For simplicity, only use a single parameter for all tools as there is barely a need to configure this flag differently per tool.

@sschuberth sschuberth requested a review from a team as a code owner February 2, 2024 11:20
@sschuberth sschuberth enabled auto-merge (rebase) February 2, 2024 11:21
@@ -97,6 +97,12 @@ pipeline {
defaultValue: ''
)

booleanParam(
name: 'SKIP_EXCLUDED',
description: 'Enable to skip scopes and paths that have been excluded via configuration.',
Copy link
Member

@fviernau fviernau Feb 2, 2024

Choose a reason for hiding this comment

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

Question: One what granularity does the "excluding paths" work? (file vs. project)

If the latter, would the comment be clearer if it said: "Enable to skip excluded scopes, projects and packages." ?

Copy link
Member Author

Choose a reason for hiding this comment

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

One what granularity does the "excluding paths" work? (file vs. project)

While of the tools are you referring to specifically?

Copy link
Member

@fviernau fviernau Feb 2, 2024

Choose a reason for hiding this comment

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

The scanner. The scanner won't skip excluded paths, which is why I proposed the alternative comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

The scanner won't skip excluded paths

For reference, that's #5018 BTW.

which is why I proposed the alternative comment.

"Enable to skip excluded scopes, projects and packages."

As packages cannot be excluded on their own either, this wording has problems as well. I'll think of something else and update the PR.

Copy link
Member

Choose a reason for hiding this comment

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

Packages can be excluded by either a scope exclude or by a path exclude matching the project's definition file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Packages can be excluded by either a scope exclude or by a path exclude

Exactly. So packages are never excluded explicitly, but only implicitly via scopes or via projects. My goal was to not document this "implictness", but focus on the first-party entities that we can explicitly exclude. I'm open for other wordings that consider this.

Copy link
Member

Choose a reason for hiding this comment

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

I believe it's clearer to speak about "what" is excluded (package, project) rather than about "how" its being excluded (scope exclude, path exclude). But I'll leave it up to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe it's clearer to speak about "what" is excluded (package, project)

In terms of ORT configuration, we have "scope excludes" and "path excludes", so to me "scope" and "path" are the "what" here, as those are the entities the user has to deal with.

While you were approving, I came up with

Enable to skip scopes and paths (and affected packages / projects) that have been configured to be excluded.

Would that be better than the current wording in your eyes?

Copy link
Member

Choose a reason for hiding this comment

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

I don't have an opinion on that one. A bit similar. But let's just do as you prefer, it's too opinionated I guess

Copy link
Member Author

Choose a reason for hiding this comment

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

I had pushed this here for reference, but decided to close it.

@sschuberth sschuberth force-pushed the jenkins-skip-excluded-parameter branch from d57cdbd to 4f37e6e Compare February 2, 2024 13:08
Enable this by default for performance reasons of showcasing ORT. For
simplicity, only use a single parameter for all tools as there is
barely a need to configure this flag differently per tool. However, the
meaning of this parameter can slightly differ per tool, as not all tools
support e.g. path excludes yet, see [1] or also [2] in that context.

[1]: #5018
[2]: #3537

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth sschuberth force-pushed the jenkins-skip-excluded-parameter branch from 4f37e6e to b33d3ad Compare February 2, 2024 13:09
@sschuberth sschuberth requested review from fviernau and a team February 2, 2024 13:09
@sschuberth sschuberth merged commit 90e9d36 into main Feb 2, 2024
19 checks passed
@sschuberth sschuberth deleted the jenkins-skip-excluded-parameter branch February 2, 2024 14:14
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