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

Add exclude_strict_matching configuration #77

Closed
wants to merge 2 commits into from

Conversation

boesing
Copy link
Member

@boesing boesing commented Mar 3, 2022

Q A
Documentation yes
Bugfix no
BC Break no
New Feature yes

Description

Added a new feature named exclude_strict_matching which actually allows projects to exclude jobs based on partially matching values.

Examples:

{
     "additional_checks": [
          {
               "name": "Do whatever",
               "job": {
                   "php": "*",
                   "dependencies": "*",
                   "command": "some command"
               }
          }
     ],
     "exclude": [
         {
             "name": "Do whatever on PHP 8."
         }
     ],
     "exclude_strict_matching": false,
}

In projects with multiple PHP versions supported, starting from PHP 7.4, every job on PHP 8.0+ will be excluded.

Prior to this change, one had to add multiple excludes with all combinations of PHP version + dependencies.

Will add additional changes to address ideas posted by @Ocramius in #76 (comment)

Added a new feature named `exclude_strict_matching` which actually allows projects to exclude jobs based on partially matching values.

Examples:

```json
{
     "additional_checks": [
          {
               "name": "Do whatever",
               "job": {
                   "php": "*",
                   "dependencies": "*",
                   "command": "some command"
               }
          }
     ],
     "exclude": [
         {
             "name": "Do whatever on PHP 8."
         }
     ],
     "exclude_strict_matching": false,
}
```

In projects with multiple PHP versions supported, starting from PHP 7.4, every job on PHP 8.0+ will be excluded.

Prior to this change, one had to add multiple excludes with all combinations of PHP version + dependencies.

Signed-off-by: Maximilian Bösing <[email protected]>
@boesing boesing force-pushed the feature/non-strict-exclusion branch from 4c8cd4d to 67f979a Compare March 3, 2022 10:14
@boesing boesing requested a review from froschdesign March 3, 2022 10:14
@@ -271,7 +282,7 @@ export default function (config) {
.filter(function (job) {
let keep = true;
config.exclude.forEach(function (exclusion) {
keep = keep && ! excludeJob(job, exclusion);
keep = keep && ! excludeJob(job, exclusion, config.exclude_strict_matching);
Copy link
Member

@internalsystemerror internalsystemerror Apr 5, 2022

Choose a reason for hiding this comment

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

I think you want a ! on the config.exclude_strict_matching here as if set to true, it sets strict_comparison to true inside excludeJob, which is the opposite of the intent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Have to double check this again but I did some integration test on my local machine and that actually worked as intended.
However, with #83 and #62, this will become pretty easy.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at around line 254 of your version of this file, if the 3rd argument is true, then it will return early and won't allow loser matchings. Unless I've missed something (very possible) this appears to be the inverse of the intent.

@boesing
Copy link
Member Author

boesing commented Jun 30, 2022

Closing this in favor of the idea of #93

@boesing boesing closed this Jun 30, 2022
@boesing boesing deleted the feature/non-strict-exclusion branch June 30, 2022 21:43
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.

3 participants