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

Added support for advanced label filters when locking #309

Closed
wants to merge 14 commits into from
Closed

Added support for advanced label filters when locking #309

wants to merge 14 commits into from

Conversation

gaspardpetit
Copy link
Contributor

Finding resources based on labels is nice, but the current implementation is very limiting. We would need to be able to filter out some resources.

Typically when implementing filters, my experience is that a trio of all/any/none is sufficient for most use cases, so instead of just adding exclusion I added all three.

  • anyOfLabels will consider resources if they have at least one matching label
  • allOfLabels will consider resources if all labels match
  • noneOfLabels will consider resources unless any of their label matches

The previously used label is still supported and considered a "simple" case of finding a resource using a single label. In implementation, this value is merged into the allOfLabels down the road.

The three new filters are placed under an "Advanced" group in the jelly.

I hope this is useful to others as well. Happy to make any changes necessary to get this PR in.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

Gaspard Petit added 3 commits February 9, 2022 17:30
Added support for advanced label filters
b5ea7ae 
Lock can now be selected with a combination of all/any/none filters. The pre-existing label is still supported to require a single specific label.
Reverted capitalization changes to toString to prevent tests from fai…
8ca6b28 
…ling
Fix for locked description
778d7f3
@jimklimov jimklimov added enhancement got conflicts Feature/fix may be reasonable, but can not be merged cleanly new parameters/features labels Mar 21, 2022
basil and others added 11 commits March 27, 2022 10:12
@basil
Enable Jenkins Security Scan
8ac464b
@basil
Use Jenkins Security Scan v2
6839cb0
@dependabot
Bump plugin from 4.33 to 4.37
faf19dc 
Bumps [plugin](https://github.com/jenkinsci/plugin-pom) from 4.33 to 4.37.
- [Release notes](https://github.com/jenkinsci/plugin-pom/releases)
- [Changelog](https://github.com/jenkinsci/plugin-pom/blob/master/CHANGELOG.md)
- [Commits](jenkinsci/plugin-pom@plugin-4.33...plugin-4.37)

---
updated-dependencies:
- dependency-name: org.jenkins-ci.plugins:plugin
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
@offa
Use String#join (jenkinsci#306)
5e91e9a
@dependabot
Bump Release Drafter from 5.18.1 to 5.19.0 (jenkinsci#314)
eb9dfd7 
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@dependabot
Bump plugin from 4.37 to 4.38
b5ae116 
Bumps [plugin](https://github.com/jenkinsci/plugin-pom) from 4.37 to 4.38.
- [Release notes](https://github.com/jenkinsci/plugin-pom/releases)
- [Changelog](https://github.com/jenkinsci/plugin-pom/blob/master/CHANGELOG.md)
- [Commits](jenkinsci/plugin-pom@plugin-4.37...plugin-4.38)

---
updated-dependencies:
- dependency-name: org.jenkins-ci.plugins:plugin
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
@basil
Use updated Jenkins Security Scan
63faa8b
Fixed reverse label null check
3f6a390 
Should have been label == null || label.isEmpty() - using StringUtils.isBlank for improved readability
Added support for advanced label filters
3ea268e 
Lock can now be selected with a combination of all/any/none filters. The pre-existing label is still supported to require a single specific label.
Merge branch 'master' into exclude-filter
e1d367d
Fixes from review
a8f8a3f 
Removed irrelavant comment, added back deprecated getResourcesWithLabel for backward compatibility
@gaspardpetit
Copy link
Contributor Author

Thanks for the review, I added back the getResourcesWithLabel with deprecation notice. I have been using this change in our production build Jenkins for over a month now and it's working great. In our case, we allow labeling some resources as 'offline' (using #305) and then filter on lock(extra:[quantity:1, allOfLabel:'console', noneOfLabel:'offline']). Happy to make any other changes that can get this into the main branch.

@chickenkiller
Copy link

I love this addition ! Any chance to see it merged soon?

@gaspardpetit gaspardpetit requested a review from jimklimov June 11, 2022 14:54
@DonnieKim411
Copy link

Any update on this? this feature will be super useful for our use case

@jimklimov jimklimov mentioned this pull request Oct 10, 2022
Closed
@mPokornyETM
Copy link
Contributor

will be handled in #428

@mPokornyETM mPokornyETM added the resource labels enhancement Improvements for resource-labels label Dec 8, 2022
@g3n35i5 g3n35i5 mentioned this pull request Jan 20, 2023
Closed
@monger39
Copy link

would this feature also support allocating a resource to a specific node (eg by name reference) , then checking as one of the conditions that such node is online? Eg image 6 nodes each having a printer, for which some printer_NN resource is defined with label 'printer'. How to lock a printer resource from an online node?

@mPokornyETM
Copy link
Contributor

would this feature also support allocating a resource to a specific node (eg by name reference) , then checking as one of the conditions that such node is online? Eg image 6 nodes each having a printer, for which some printer_NN resource is defined with label 'printer'. How to lock a printer resource from an online node?

There are few similar PRs and no one is completed (for some historical reason). At the moment it is hard to merge master in this branches.
Therefore will be this PR probably closed and handled in #428.
To your question about node filters.
I have also created #341 which is similar idea.
When you need to use some special (more sophisticated) filter just now, you can use a Groovy expression. When it does not match your requirements, please create a new enhancement issue here in GitHub. It will be also helpful to provide a small example, how you want to use it in your pipeline?

PS: Strange for some reason is the Groovy expression visible only in free-style jobs. This shall be fixed. Please help me and create new issue here in GitHub (thx)
image

PS2: currently I am thinking about new global-lockable-resources-shared-library which extends core functionality in groovy. Because of historical reason, are many things no more possible here (without big effort, by refactoring this plugin). This can be loaded by you, in your Jenkins instance and used for free. BUT it need time and we shall specified which content shall be done there. Currently nobody will contribute (there)[https://github.com//discussions/445]. Maybe you will find some time and help me (us). It does not mean to contribute on coding. Describe an idea in new issues helps too ;-)

@monger39 monger39 mentioned this pull request Jan 31, 2023
Open
@monger39
Copy link

@mPokornyETM : I have created a new issue #455 describing my idea of the use-case. I would be even willing to help on the coding part, but am an absolute newbie on plugin development (and Java ;-) )

@mPokornyETM
Copy link
Contributor

@mPokornyETM : I have created a new issue #455 describing my idea of the use-case. I would be even willing to help on the coding part, but am an absolute newbie on plugin development (and Java ;-) )

Any help is welcome. Contributing is not only about Java.
Documentation, testing, PR-reviews, bug-management, localizations, bring new ideas and many more.
Please join our chat and we can discuss any thing you want ;-)
https://gitter.im/jenkinsci/lockable-resources

@mPokornyETM mPokornyETM added the Triage Need to clarify, remove, close or whatever to clean up open issues / PRs label Feb 1, 2023
@mPokornyETM mPokornyETM added this to the Triage milestone Feb 1, 2023
@mPokornyETM mPokornyETM mentioned this pull request Mar 23, 2023
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimklimov jimklimov Awaiting requested review from jimklimov

Assignees
No one assigned
Labels
enhancement got conflicts Feature/fix may be reasonable, but can not be merged cleanly new parameters/features resource labels enhancement Improvements for resource-labels Triage Need to clarify, remove, close or whatever to clean up open issues / PRs
Projects
None yet
Milestone
Triage
Development

Successfully merging this pull request may close these issues.
8 participants
@gaspardpetit @chickenkiller @DonnieKim411 @mPokornyETM @monger39 @jimklimov @basil @offa