-
Notifications
You must be signed in to change notification settings - Fork 164
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 nodes by label step #39
Conversation
} | ||
|
||
@Override | ||
protected ArrayList<String> run() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should not be that type specific here. List<String>
is better.
@Override | ||
@NonNull | ||
public String getDisplayName() { | ||
return "List of node names by their label"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I18n
If I grok the logic below this statement is not exactly true, since it only lists nodes that are online.
A potential enhancement could be to add an optional parameter allowing the caller to not filter out based on online state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it filters on the online state. So the optional filter would be great 👍
#36 Is now merged, so you should target |
@rsandell rebased onto master and added your suggestion. |
I forgot to ask you to update |
We have this step in our shared library.
The step enables us to maintain a group of nodes through Jenkins by using their label to get the desired group, ie. 'MacOS' label.
Currently, our shared library step is using the Jenkins JSON API, earlier it was using unsafe groovy methods.
So I suggest we add the step to this plugin 👍
The suggested name for the step is 'nodesByLabel' However I am open for better suggestion, also the package location.
Use case: