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

Don't force the use of underscore to separate slave prefix from number. #92

Closed
wants to merge 1 commit into from
Closed

Conversation

joshk0
Copy link
Contributor

@joshk0 joshk0 commented Apr 24, 2018

  • Underscores complicate things when you want to create DNS records for
    the ephemeral slaves. Underscores are not valid characters for DNS :(
  • The user can always add the underscore back in their config.

This should be documented as a breaking functional change but I don't
really see that people would be designing tons of functionality around
this underscore. As a result, there is no migration from pre- to post-
removal versions of the plugin.

* Underscores complicate things when you want to create DNS records for
  the ephemeral slaves.
* The user can always add the underscore back in their config.

This should be documented as a breaking functional change but I don't
really see that people would be designing tons of functionality around
this underscore. As a result, there is no migration from pre- to post-
removal versions of the plugin.
@joshk0
Copy link
Contributor Author

joshk0 commented May 8, 2018

@pjdarton could you please have a look?

@pjdarton
Copy link
Member

pjdarton commented May 8, 2018

Yes, it's on my list of things to do. I've been out of the office for a week (and was busy the previous week) but, yes, it's on the list - I'm delaying, not ignoring you entirely ;-)

@pjdarton
Copy link
Member

Hi. Sorry for the delay - I had a load of issues with the docker-plugin I needed to fix - but I'm back on the case now and I've had a look at this.
I have good news and I have bad news.

The bad news is that I can't merge this as-is.
I can see that the code changes do achieve exactly the effect you were after (which is good), but this PR doesn't include all the other bits that are needed when making changes that affect the configuration / the way the configuration is used.
e.g. if I merged this & released a new version of the plugin, and then someone upgraded their plugin, they'd "suddenly" find that their node names were missing the underscore, and that might be a breaking change for some users.
We have to ensure that, when upgrading the plugin, "nothing changes" by default.

The good news is that the extra changes required aren't too arduous and so I've come up with #93 which includes additional changes that address my concerns. I've (very briefly) tried it out to verify that the configuration upgrade code works as I expect, but it will need testing properly before release and I'd like to enlist your help on that front.

FYI the extra changes ensure that, if someone upgrades from an earlier version of the plugin then:

  • the underscore is no longer hard-coded but now appears in the template configuration.
  • that underscore can be removed from the template configuration.
  • the online help no longer says that there's always an underscore added.

Finally, can you please test #93 and let me know what you think.
You'll find a .hpi file to download from the artifacts section on the ci.jkenkins.io site that's linked to from the PR's "all checks" section.

@pjdarton pjdarton added WIP Not ready for merge. enhancement Adds extra functionality labels May 14, 2018
@pjdarton pjdarton self-assigned this May 14, 2018
@pjdarton
Copy link
Member

pjdarton commented Jun 4, 2018

Superseded by #93
All changes in here are in there.
Closing this one. All further discussion to be done in #93 instead.

@pjdarton pjdarton closed this Jun 4, 2018
pjdarton added a commit that referenced this pull request Jun 5, 2018
Stop hard-coding underscore in clone names.
This includes changes from #92 by @joshk0 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adds extra functionality WIP Not ready for merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants