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

Add descriptor visibility filter #112

Merged
merged 3 commits into from
Nov 5, 2019
Merged

Add descriptor visibility filter #112

merged 3 commits into from
Nov 5, 2019

Conversation

jetersen
Copy link
Member

@jetersen jetersen commented Nov 5, 2019

No description provided.

@pjdarton
Copy link
Member

pjdarton commented Nov 5, 2019

My concern with these is that the visibility filter is saying "These must not be shown to anyone" whereas the truth is that they should be an option for vSphere cloud-provisioned slaves (but only vSphere cloud-provisioned slave nodes).
I suspect that the @CheckForNull Object context argument is intended to be used to allow visbility filters to say "we're only visible to vSphere cloud-provisioned slave nodes".

...that said, I think this would only ever become a problem if vSphereCloudSlaveTemplate.DescriptorImpl.getRetentionStrategyDescriptors() was re-coded to ask Jenkins to list all appropriate retention strategies instead of hard-coding that list tho' ... so we can get away with this simple implementation for now 😁

If you agree with that, and you're happy for this to be merged, please add a PR description and I'll merge it.

@pjdarton
Copy link
Member

pjdarton commented Nov 5, 2019

I suspect an end-of-line issue - the diff is now showing you've replaced the entire contents of both files...

@jetersen
Copy link
Member Author

jetersen commented Nov 5, 2019

😱 I just used GitHub to merge a simple conflict 😭

@pjdarton
Copy link
Member

pjdarton commented Nov 5, 2019

Of course, fixing the EOLs in #114 has meant this now has merge issues ... again 😀

@pjdarton pjdarton merged commit 911b50c into jenkinsci:master Nov 5, 2019
@jetersen
Copy link
Member Author

jetersen commented Nov 5, 2019

i'll double check on the visibility but I recall them as being visible and selectable :)

@pjdarton
Copy link
Member

pjdarton commented Nov 5, 2019

Thank you for your perseverance 👍

@jetersen jetersen deleted the fix/visiblityFilter branch November 5, 2019 18:38
@pjdarton
Copy link
Member

pjdarton commented Nov 5, 2019

PS. It's the end of my working day now, but if you find any other issues, comment/raise-a-PR etc and I'll be back online in around 15 hours.

@jetersen
Copy link
Member Author

jetersen commented Nov 5, 2019

Just tested with the latest incremental build from master. Visibility filter works just fine:

image

This is a cloud provisioned one 😄

@pjdarton
Copy link
Member

pjdarton commented Nov 6, 2019

For a cloud-provisioned slave, none of that should be editable anyway... They should be edited by configuring the template that defines how they're created, but that's (probably) a bug unrelated to your changes - one of many minor bits of tech-debt in this plugin code.

The important things are that:

  1. When creating or editing a (vSphere slave) template, the vSphere retention strategies are available to be selected. I think that code has a hard-coded list of just the two vSphere options and doesn't pay any attention to visibility filters, so that should be unaffected.
  2. When creating a new non-cloud vSphere slave, the vSphere retention strategies are available to be selected.
  3. When creating any other kind of slave, the vSphere retention strategies are not available.
  4. When editing a previously-created slave, the same retention strategies are listed as were available when it was created.

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.

2 participants