Skip to content

Assign properties to lockable resources and retrieve them in environment variables#295

Closed
gaspardpetit wants to merge 12 commits intojenkinsci:masterfrom
eidosmontreal:add-properties
Closed

Assign properties to lockable resources and retrieve them in environment variables#295
gaspardpetit wants to merge 12 commits intojenkinsci:masterfrom
eidosmontreal:add-properties

Conversation

@gaspardpetit
Copy link
Copy Markdown
Contributor

This is pretty much #20 updated for HEAD - most of the credits go to @McFoggy

I recommend reviewing this after #294 since it will simplify the complexity of this PR. #294 adds an environment variable for each acquired locks when unlocking multiple ones, and this PR builds on this change.

The lock properties can be set on the lock definition in the System Configuration;
When setting a variable, the name of the variable is used as a prefix to also set an environment variable for each of the lockable resource's properties. For example:

lock(label: 'some_resource', variable: 'LOCKED_RESOURCE', quantity: 2) {
  // comma separated names of all acquired locks
  echo env.LOCKED_RESOURCE

  // first lock
  echo env.LOCKED_RESOURCE0
  echo env.LOCKED_RESOURCE0_PROP_ABC

  // second lock
  echo env.LOCKED_RESOURCE1
  echo env.LOCKED_RESOURCE0_PROP_ABC
}

We plan on using these properties to store information such as the IP, URL or an account to use with the lockable resource obtained.

  • 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

@jimklimov
Copy link
Copy Markdown
Contributor

Thanks for the revamp, looks nice. Is there some scripted/declarative pipeline way of setting (adding, changing, removing) properties for a resource, e.g. if making an ephemeral one? I see the JCasC example but not any (maybe missed?) for "active" code...

@jimklimov
Copy link
Copy Markdown
Contributor

After #293, can you please revise the merge-conflicts?

@jimklimov jimklimov added got conflicts Feature/fix may be reasonable, but can not be merged cleanly enhancement extended properties Ideas about more aspects of the lockable resource instances than we had before labels Feb 6, 2022
@jimklimov
Copy link
Copy Markdown
Contributor

#294 also merged, can you please rebase on top of your improvements there? and thanks! :)

@jimklimov
Copy link
Copy Markdown
Contributor

Also wondering: there are several requests over time like #43 and #141 about saving a track of resource's usage. I suppose there could be a Map (by timestamp?) for builds that locked/unlocked the resource and users who reserved/unreserved it. Would these properties from the PR here be a decent way of tracking that (and/or setting the length limit of the map, so the audit trail does not eat all storage eventually), or should that better be stored in some dedicated fields?

@gaspardpetit
Copy link
Copy Markdown
Contributor Author

Is there some scripted/declarative pipeline way of setting (adding, changing, removing) properties for a resource, e.g. if making an ephemeral one? I see the JCasC example but not any (maybe missed?) for "active" code...

No - to be honest this is my first contribution on a jenkins plugin. I am a long time user, long time java developer, so not completely lost, but still ramping up on how plugins work.

I do like the idea of being able to alter lock properties from a pipeline, this could even extend the use cases of locks to state machines within jenkins: imagine if we could get a pipeline trigger on a lock having certain labels.

Pipeline A creates a lock with label "init"
Pipeline B triggers on a lock having the label 'init', grabbing it on start, does some work with it, changes properties and sets the label to "ready"
Pipeline C triggers on a lock having the label 'ready', fetches the lock properties to continue the work on it
etc.

This could open up new ways of coordinating complex jobs in Jenkins...

I don't mind implementing those, can you point me to an example? Should I look at the root actions such as the recent doSteal?

Would these properties from the PR here be a decent way of tracking that (and/or setting the length limit of the map, so the audit trail does not eat all storage eventually), or should that better be stored in some dedicated fields?

I would vote for dedicated fields. These are computed and not needed as env variables. In my opinion, properties have a very specific purpose: they hold key-values defined by the users for the users. If we have use cases for built-in properties, I would keep them separate.

@jimklimov
Copy link
Copy Markdown
Contributor

I am not sure about pipelines triggering due to a lock change. Probably could be done, but needs identification of what jobs react to what resources, and might generally not be in scope of this plugin per se, but of some new plugin requiring this one for parts of its job and requiring others for JobA-causes-JobB stuff.

Maybe same logic can be left up to users, with their pipelines coded to trigger others and passing the lockable resource as argument or otherwise using the modifiable label matches.

Thinking of it, by the way, the edition of labels (at least via LRM... maybe can hook back to default-Jenkins.get().LRM.get() too, for direct LR object actions?) should reset the cachedCandidates queues for that resource instance per #171 now that it's merged (so improper resources are no longer cached as suitable candidates). I think I only amended that for unlock and unreserve activities.

offa and others added 4 commits February 6, 2022 16:02
Also follow Google Java Code Style identation style
Use the `variable` parameter to prefix lock attributes from each acquired locks
@gaspardpetit
Copy link
Copy Markdown
Contributor Author

Sorry I got caught in merge issues because of the whitespace changes (at least I have myself to blame for that) and it was easier to just re-apply on a fresh branch. I am closing this PR and will continue conversation in #299

@gaspardpetit gaspardpetit deleted the add-properties branch February 6, 2022 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement extended properties Ideas about more aspects of the lockable resource instances than we had before got conflicts Feature/fix may be reasonable, but can not be merged cleanly variables exposed by lock actions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants