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

Lockable priority strategy #632

Merged
merged 35 commits into from
Mar 19, 2024
Merged

Conversation

mPokornyETM
Copy link
Contributor

@mPokornyETM mPokornyETM commented Feb 26, 2024

fix #560
fix #223
fix #201
fix #81

Testing done


Inverspe precedence

start time | job | resource | inversePrecedence
------ |--- |--- |---
00:01 | j1 | resource1 | false
00:02 | j2 | resource1 | false
00:03 | j3 | resource1 | true
00:04 | j4 | resource1 | false
00:05 | j5 | resource1 | true
00:06 | j6 | resource1 | false

  expected lock order: j1 -> j5 -> j3 -> j2 -> j4 -> j6

lock priority

  start time | job | resource  | priority
  ------     |---  |---        |---
  00:01      | j1  | resource1 | 0
  00:02      | j2  | resource1 | <default == 0>
  00:03      | j3  | resource1 | -1
  00:04      | j4  | resource1 | 10
  00:05      | j5  | resource1 | -2
  00:06      | j6  | resource1 | 100

  expected lock order: j1 -> j6 -> j4 -> j2 -> j3 -> j5

Inverse precendense is not combinable with priority !


Manual tests also done with some priority as string (no-a-number)

Proposed upgrade guidelines

N/A

Localizations

  • English
  • German
  • French
  • Slovak
  • Czech
  • ...

Submitter checklist

  • The Jira / Github issue, if it exists, is well-described.
  • The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
    • The changelog generator for plugins uses the pull request title as the changelog entry.
    • Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during the upgrade.
  • There is automated testing or an explanation that explains why this change has no tests.
  • New public functions for internal use only are annotated with @NoExternalUse. In case it is used by non java code the Used by {@code <panel>.jelly} Javadocs are annotated.
  • New or substantially changed JavaScript is not defined inline and does not call eval to ease the future introduction of Content Security Policy (CSP) directives (see documentation).
  • For dependency updates, there are links to external changelogs and, if possible, full differentials.
  • For new APIs and extension points, there is a link to at least one consumer.
  • Any localizations are transferred to *.properties files.
  • Changes in the interface are documented also as examples.

@mPokornyETM mPokornyETM added bug enhancement java Pull requests that update Java code lock queue priority and visibility roles and permissions Who may manage what? ui Features that may impact UI, pages made by the plugin or external UIs (BO, legacy, etc.) documentation log messages new parameters/features labels Mar 3, 2024
@mPokornyETM mPokornyETM added the Need test Automated tests shall be extended. label Mar 4, 2024
@mPokornyETM
Copy link
Contributor Author

The feature is more or less complete, I will provide more tests, screenshots and documentation as well.

@mPokornyETM mPokornyETM changed the title Queue-sorter Lockable priority strategy Mar 4, 2024
Copy link
Contributor

@PayBas PayBas left a comment

Choose a reason for hiding this comment

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

Can confirm that inversePrecedence now works as #560 (comment) scenario 2.

So that's definitely a step in the right direction.
The new "Queue priority" feature still feels very rough though and will need some polish.

@mPokornyETM
Copy link
Contributor Author

@PayBas thx for feadback. I will try to implement it ASAP

Copy link
Contributor

@PayBas PayBas left a comment

Choose a reason for hiding this comment

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

Functionality looks good. Didn't find any issues.
Only some minor cosmetic suggestions.

Also note that https://plugins.jenkins.io/data-tables-api/releases/ 2.0 has been released, which breaks quite a bit of the Lockable Resources table.

I had to downgrade the DataTables plugin to v1.13.8-4 in order to properly test/review this PR.

@PayBas
Copy link
Contributor

PayBas commented Mar 14, 2024

LGTM 👍

@mPokornyETM
Copy link
Contributor Author

LGTM 👍

give me a little bit. I will provide few more automatic tests (for queue management)

@mPokornyETM mPokornyETM merged commit 28be4cc into jenkinsci:master Mar 19, 2024
16 checks passed
@mPokornyETM mPokornyETM removed work-in-progress Need test Automated tests shall be extended. labels Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug dependencies Dependeny updates documentation enhancement java Pull requests that update Java code localization lock queue priority and visibility log messages new parameters/features ready for review PR is ready for review roles and permissions Who may manage what? test ui Features that may impact UI, pages made by the plugin or external UIs (BO, legacy, etc.)
Projects
None yet
2 participants