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

Added node usage mode to the templates #338

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

ajrussellsap
Copy link

Hello,

Firstly, thanks for creating this awesome Jenkins plugin! My team relies on it heavily as it allows us to use our resources in a much more efficient way.

One feature we would like though, is the ability to set the node usage mode in the templates so that we can have certain nodes that will only be used by jobs that specify the relevant label. Therefore, I have created this PR which adds this feature. I notice that it is also mentioned in the following issue:

#199

The change is relatively simple but here is a quick summary:

  • Added the Usage field to the General Configuration section of the template. I decided to add it here rather than in the Provisioning Details section because it fundamental to whether a node will get provisioned and it works in conjunction with the Labels field.
  • The mode field has been added to the JCloudsSlaveTemplate class but it will not fail if it is not set - it will default to NORMAL so the behaviour will not change when updating to this version of the plugin.
  • The mode field is now a parameter of the constructor of the JCloudsSlave class.
  • Updated the unit tests to set and check the new mode field.
  • Updated the documentation to mention the new field and also replaced the existing screenshots.

As well as the updates to the unit tests, I have also done a lot of manual testing of the new plugin and it is working well on my Jenkins server.

Anyway, I would be extremely grateful if you could have a look at my PR with a view to merging it. Please also let me know if there are any updates to the PR that you would like to see.

Many thanks

Alex

  • 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

Copy link
Member

@olivergondza olivergondza left a comment

Choose a reason for hiding this comment

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

Thanks, this is much desired change. I have made some mostly cosmetic review comments. However, the big one would be, to move the feature from the template itself to SlaveOptions.

@ajrussellsap
Copy link
Author

Hi @olivergondza

Thanks for reviewing my PR. I have made the code changes you recommended.
Regarding moving the Usage field to SlaveOptions: the reason I put it in the main template was because it is used in conjunction with the Labels field to determine whether the template will be used. Therefore, I thought it made most sense for those two fields to be adjacent in the UI. I also looked at ec2-plugin, google-compute-engine-plugin and oracle-cloud-infrastructure-compute-plugin and they all have those two fields adjacent to each other. Of course, it is just my opinion so I will move the new field if that is what you really want but I just wanted to state the case for keeping Usage in the template.

Thanks

Alex

@ajrussellsap
Copy link
Author

Hi @olivergondza

Have you seen my previous comment about the location of the Usage field in the UI? It would be great if you could clarify that you think the Usage field should be accessed via the Provisioning Details button. Personally I don't think so because the field is fundamental to the template being used but I am happy to discuss :)

Thanks

@olivergondza
Copy link
Member

Thanks for reflecting that. I do see that JCloudsSlaveTemplate is utilizing the mode field on a several occasions, though I still do believe this belongs to SlaveOptions. So it can be inherited/overridden as the other node attributes.

Can you move it there? I believe we are almost done here.

@ajrussellsap
Copy link
Author

Hi @olivergondza

Sorry for the delay. I have now moved the mode field to SlaveOptions. Let me know if there is anything else you think needs doing.

Thanks

@ajrussellsap
Copy link
Author

Hi @olivergondza

I think this PR contains all of the changes that you requested. Are you able to look at merging it? Please let me know if there is anything else you would like done.

Thanks

@pbusko
Copy link

pbusko commented May 13, 2024

would love to see this feature merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants