-
-
Notifications
You must be signed in to change notification settings - Fork 26
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
Jenkins node helper API #1
Conversation
- Add machines to Jenkins: - Set machine description based on machine stats - Determine machine capabilities - Add labels to nodes based on machine capabilities
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.
This looks good overall! LMK if you'd like me to merge (would be good if you raised the issues etc so we know what to work on though).
|
||
class NodeHelper { | ||
|
||
// TODO: Move strings out to a config file |
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.
Raise an issue for this please.
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.
|
||
String ret = "INVALID_NODE_NAME"; | ||
|
||
if (newNodeName.length() > 2) { // TODO: Some sort of validation for node names |
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.
Add an issue for the TODO
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.
def launcher | ||
) { | ||
|
||
String ret = "INVALID_NODE_NAME"; |
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.
ret --> nodeName?
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.
Same goes for all usages of 'ret', can replace with more meaningful variable name
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.
Using "ret" makes it easier to follow the return variable through a function. As for meaning, the function name, header comments, and function(s) called to update "ret" variable aid in the meaning.
* @return string of labels set on the computer | ||
*/ | ||
public String addLabel(String computerName, String label) { | ||
String ret = "addLabel:COMPUTER_NOT_FOUND"; |
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.
ret --> label?
* @return label string | ||
*/ | ||
public String constructLabels(String computerName) { | ||
/* Procedure for adding support for new labels |
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.
Haul comment up
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.
I have left that comment inside because it's for anyone who wants to add more functionality to the function. It isn't necessarily needed in the header comment of the function
if (computer != null) { | ||
ret = constructOsLabel(computer.getName()); | ||
if (getOsArch(computer.getName()).contains("ppc") | ||
&& getOsKernelInfo(computer.getName()).get(0).equalsIgnoreCase("linux")) { |
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.
could extract this to a helper function
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.
Done! I'll add it to my next commit
|
||
Computer computer = getComputer(computerName); | ||
if (computer != null) { | ||
ret = "hw.hypervisor.";// TODO: finish implementation, get something |
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.
Add issue in issue tracker
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.
ret = getAIXOsInfo(computer); | ||
break; | ||
case "zos": | ||
// ret = // TODO: find the command |
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.
Issue
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.
Done! z/OS version
// ret = // TODO: find the command | ||
// break; | ||
case "mac": | ||
// ret = getMacOsInfo(computer); |
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.
commented out on purpose?
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, wasn't sure if we needed the version number since it is the same as kernel version. I implement it and check it in with the next commit
* this returns an inaccurate result. | ||
* This has to be fixed by the person who did the setup for the virtual | ||
* machines. | ||
* As of right now, we do not know how to fix the bug. |
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.
TODO this
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.
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.
This was actually bad on my part. I haven't had a chance to check this bug on external machines. I'll run tests regarding this on the external machines and open an issue if needed.
@VermaSh Just wanted to say thanks for opening up those issues - appreciate the admin effort! |
The class NodeHelper partially addresses feature requests in adoptium/infrastructure#259
Add machines to Jenkins: addNewNode()
Determine machine stats
Set machine description based on machine stats
Determine machine capabilities
Add labels to nodes based on machine capabilities
Other useful functions
Few side notes:
At the moment it's very basic but it can be extended to enforce a standardized naming convention