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
Fix ist slaves #43
Fix ist slaves #43
Conversation
6cea7e0
to
6afc681
Compare
@@ -189,7 +189,8 @@ private synchronized void onInitialImageStream(ImageStreamList imageStreams) { | |||
} | |||
// for IST, can't set labels, so check annotations | |||
if (ist != null && hasSlaveLabelOrAnnotation(ist.getMetadata().getAnnotations())) { | |||
results.add(this.podTemplateFromData(ist.getMetadata().getName(), | |||
// note, pod names cannot have colons | |||
results.add(this.podTemplateFromData(ist.getMetadata().getName().replaceAll(":", "."), |
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.
so now "my.imagestream:foo" and "my:imagestream.foo" are going to get mapped to the same thing. are dots allowed in imagestream names and tag names? i suspect they are.
unfortunately this is hard problem for any separator unless there's a value that's valid in pod names and not valid in imagestreams or tag names.
what was valid for pod names seemed pretty limited .... lower case
aphanumeric, hyphens, and periods ... that was it
according to the validation error message that came back
…On Tue, Jun 20, 2017 at 4:22 PM, Ben Parees ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/main/java/io/fabric8/jenkins/openshiftsync/ImageStreamWatcher.java
<#43 (comment)>
:
> @@ -189,7 +189,8 @@ private synchronized void onInitialImageStream(ImageStreamList imageStreams) {
}
// for IST, can't set labels, so check annotations
if (ist != null && hasSlaveLabelOrAnnotation(ist.getMetadata().getAnnotations())) {
- results.add(this.podTemplateFromData(ist.getMetadata().getName(),
+ // note, pod names cannot have colons
+ results.add(this.podTemplateFromData(ist.getMetadata().getName().replaceAll(":", "."),
so now "my.imagestream:foo" and "my:imagestream.foo" are going to get
mapped to the same thing. are dots allowed in imagestream names and tag
names? i suspect they are.
unfortunately this is hard problem for any separator unless there's a
value that's valid in pod names and not valid in imagestreams or tag names.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#43 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADbadNcGxFG0xoECSWhIRDyDh_Fb39l1ks5sGCoRgaJpZM4OAEsV>
.
|
yeah most of this stuff accepts anything that's valid for a DNS name. there are options to pick a more distinctive separator that's less likely to collide, but i admit even my scenario is relatively contrived. |
this commit passed tests in openshift repo ... given where the separator discussion landed with @bparees going to merge the jenkins ci jobs are failing with some maven complaint regarding our pom's use of jdk 1.8 when the dependency of the k8s plugin was pulled in by Ryan, though will circle back to see if the pom.xml can be tweaked to clean those up after we cycle through last minute 3.6 bugs |
Fix ist slaves
@bparees see openshift/origin#14756 (comment)
also i inadvertently cut a new release before merging this change .... i'll have to cut a 0.1.22 after 0.1.21 is complete