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

Update to v1309 changes LRM API and behavior #712

Open
jimklimov opened this issue Sep 25, 2024 · 1 comment
Open

Update to v1309 changes LRM API and behavior #712

jimklimov opened this issue Sep 25, 2024 · 1 comment
Labels
bug Clarification needed Additional clarification needed

Comments

@jimklimov
Copy link
Contributor

jimklimov commented Sep 25, 2024

Jenkins and plugins versions report

Environment
Paste the output here

What Operating System are you using (both controller, and any agents involved in the problem)?

Linux, Windows
Jenkins LTS 2.462.2
LR plugin v1309 .. v1315

Reproduction steps

Using the plugin for a long time from custom JSL code by "directly" calling methods of the LockableResourceManager and LockableResource class instances. Recent changes in #673 removed the previously deprecated methods to unlock() the resources, dissociating them from a build, renaming them into unlockResources(). Technically, they were deprecated. But dropping them as a side casualty of other refactoring, unannounced, was not too friendly (I'd prefer complaining aliases that refer to new methods transparently) :)

Another related issue is that the newly named methods are not complete replacements: they now require a non-null value for a build object - specifically the one the resource is locked for, whereas previously passing a null allowed to just free it (e.g. passing the same resource between jobs and builds and not caring much who grabbed it first). To this effect, just noting that we can get the build object from the resource object itself, so it is a work-aroundable complication. Not sure why it had to happen in the first place though :)

def lrm = LockableResourcesManager.get()
def lr = lrm.fromName('testSUT-1')
lrm.unlockResources([lr], lr.getBuild())

Alternately, lrm.unlockResources([lr]) without a second argument should also work - assuming all listed resources were locked by the same build (first resource's build is applied to everyone's unlocking).

Expected Results

Old scripts using the plugin continue working, or the "catastrophic" change of API is loudly announced.

Actual Results

  • Old scripts fail to unlock resources due to missing methods
  • The amended code is slightly more complex to provide the correct build object extracted from the same resource we are freeing

Anything else?

For completeness, just in case someone else stumbles on this - the API change with PR #673 included the following removed methods in LockableResourcesManager.java :

  • getResourcesFromBuild()

  • getReadOnlyResources() - concept obsoleted, was a speedup for what is not a problem now => plain getResources() should suffice

  • freeResources() - API change (build no longer @Nullable)

  • unlock() - several (all?) method signatures removed; roughly replaced by new unlockResources() methods (tied to a build that may not be null now)

  • unlockNames() - one method signature removed, another changed (build no longer @Nullable)

  • added unlockBuild()

Are you interested in contributing a fix?

No response

@jimklimov jimklimov added the bug label Sep 25, 2024
@mPokornyETM
Copy link
Contributor

A was expecting issues like this. All the changes was done, that the multi thread are safe (concurent exception things) and the performance is OK.

I know, it is frustrated ti change all the groovy scripts. We have the same work to do in our company.
But, it was not possible to keep all the old methods, and everthink works as before.

I am realy SORRY for that.

Anyway, I have one question.
Why do you need to unlock resources by groovy script? This migth leads to very unhappy scenarios.

ps: something like this will be really helpfull
https://community.jenkins.io/t/how-to-handle-with-end-user-groovy-shared-libraries/22558

@mPokornyETM mPokornyETM added the Clarification needed Additional clarification needed label Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Clarification needed Additional clarification needed
Projects
None yet
Development

No branches or pull requests

2 participants