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.

Sign up for GitHub

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

Jump to bottom

Pod annotations cannot contain duplicate keys when combining pod templates #220

Merged
merged 2 commits into from
Sep 11, 2017
Merged

Pod annotations cannot contain duplicate keys when combining pod templates #220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
332f331
pod annotations must not contain duplicate keys
Sep 10, 2017
eed603c
Merge branch 'master' into patch-1
Sep 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodAnnotation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,22 @@ public void setValue(String value) {
this.value = value;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;

PodAnnotation that = (PodAnnotation) o;

return key != null ? key.equals(that.key) : that.key == null;

}

@Override
public int hashCode() {
return key != null ? key.hashCode() : 0;
}

@Extension
@Symbol("podAnnotation")
public static class DescriptorImpl extends Descriptor<PodAnnotation> {
Expand Down
7 changes: 3 additions & 4 deletions src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import hudson.tools.ToolLocationNodeProperty;

public class PodTemplateUtils {

/**
* Combines a {@link ContainerTemplate} with its parent.
* @param parent The parent container template (nullable).
Expand Down Expand Up @@ -89,11 +88,11 @@ public static PodTemplate combine(PodTemplate parent, PodTemplate template) {
String nodeSelector = Strings.isNullOrEmpty(template.getNodeSelector()) ? parent.getNodeSelector() : template.getNodeSelector();
String serviceAccount = Strings.isNullOrEmpty(template.getServiceAccount()) ? parent.getServiceAccount() : template.getServiceAccount();
Node.Mode nodeUsageMode = template.getNodeUsageMode() == null ? parent.getNodeUsageMode() : template.getNodeUsageMode();

Set<PodAnnotation> podAnnotations = new LinkedHashSet<>();
podAnnotations.addAll(parent.getAnnotations());
podAnnotations.addAll(template.getAnnotations());

podAnnotations.addAll(parent.getAnnotations());
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I am wrong, but this looks like the precedence favors parent, not as you write, template:

changed precedence so that the template will override the parent in inheritance.

Why do you want to change the precedence in the first place? Reading the documentation, I would expect the template to have precedence.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the same but actually he is right

https://docs.oracle.com/javase/7/docs/api/java/util/HashSet.html#add(E)

Adds the specified element to this set if it is not already present.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, makes sense when you think about it.


Set<PodImagePullSecret> imagePullSecrets = new LinkedHashSet<>();
imagePullSecrets.addAll(parent.getImagePullSecrets());
imagePullSecrets.addAll(template.getImagePullSecrets());
Expand Down
9 changes: 6 additions & 3 deletions src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,15 @@ public class PodTemplateUtilsTest {

private static final PodAnnotation ANNOTATION_1 = new PodAnnotation("key1", "value1");
private static final PodAnnotation ANNOTATION_2 = new PodAnnotation("key2", "value2");
private static final PodAnnotation ANNOTATION_3 = new PodAnnotation("key1", "value3");

private static final ContainerTemplate JNLP_1 = new ContainerTemplate("jnlp", "jnlp:1");
private static final ContainerTemplate JNLP_2 = new ContainerTemplate("jnlp", "jnlp:2");

private static final ContainerTemplate MAVEN_1 = new ContainerTemplate("maven", "maven:1", "sh -c", "cat");
private static final ContainerTemplate MAVEN_2 = new ContainerTemplate("maven", "maven:2");


@Test
public void shouldReturnContainerTemplateWhenParentIsNull() {
ContainerTemplate result = combine(null, JNLP_2);
Expand Down Expand Up @@ -152,14 +154,15 @@ public void shouldCombineAllAnnotations() {
PodTemplate parent = new PodTemplate();
parent.setName("parent");
parent.setNodeSelector("key:value");
parent.setAnnotations(asList(ANNOTATION_1));
parent.setAnnotations(asList(ANNOTATION_1, ANNOTATION_2));

PodTemplate template1 = new PodTemplate();
template1.setName("template");
template1.setAnnotations(asList(ANNOTATION_2));
template1.setAnnotations(asList(ANNOTATION_3));

PodTemplate result = combine(parent, template1);
assertEquals(2, result.getAnnotations().size());
assertEquals("value3", result.getAnnotations().get(0).getValue().toString());
}


Expand Down