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

Pod Template "Annotations" Field #105

Merged
merged 6 commits into from
Feb 12, 2017

Conversation

austinmoore-
Copy link
Contributor

Adds the ability to specify a list of annotations to create the pod with.

Copy link
Contributor

@carlossg carlossg left a comment

Choose a reason for hiding this comment

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

Thanks! You also need to update https://github.com/jenkinsci/kubernetes-plugin/blob/master/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStep/config.jelly

And it helps reviewing changes if formatting (space changes) is done separately

@austinmoore-
Copy link
Contributor Author

Whoops, sorry about those whitespace changes. I'll clean those up, and add the jelly soon. Thanks!

@austinmoore-
Copy link
Contributor Author

@carlossg Hey sorry this took so long. I completely forgot that I had made this PR until about a week ago. Anyway, I removed those whitespace changes so this PR should be easier to review now. I've also added an annotation element to the jelly. LMK if there's anything else.

@dbirtch-va
Copy link

This is awesome, we're looking for this exact feature set in order to use annotations to specify pod anti-affinity to better spread our jenkins builds over nodes with attached SSD's.

Copy link
Contributor

@carlossg carlossg left a comment

Choose a reason for hiding this comment

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

Added one comment and needs a rebase to fix conflicts

@@ -56,6 +56,7 @@ public boolean start() throws Exception {
newTemplate.setContainers(step.getContainers());
newTemplate.setNodeSelector(step.getNodeSelector());
newTemplate.setServiceAccount(step.getServiceAccount());
newTemplate.setAnnotations(step.getAnnotations());
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI when upgrading getAnnotations() is going to be null, because is loaded from xml config where it doesn't exist, have you checked that the API won't break with a null value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was unaware of that. I just did a test where I upgraded from 0.7, to my version, and all the data remained.

I believe it worked because I declare annotations here with an initial value: https://github.com/jenkinsci/kubernetes-plugin/pull/105/files#diff-37ffb9809d7b186d5733b266c664ba41R36

Copy link
Contributor

Choose a reason for hiding this comment

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

the initial value won't be used when loading the xml config from disk, so maybe it's better if you explicitly add a readResolve that sets annotations to empty list if it is null

As an example
https://github.com/jenkinsci/kubernetes-plugin/blob/master/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java#L446

@austinmoore-
Copy link
Contributor Author

In the most recent commits I've added a null check to readResolve, and rebased onto master.

@carlossg carlossg merged commit 9986b4b into jenkinsci:master Feb 12, 2017
@carlossg
Copy link
Contributor

thanks!

@pawelprazak
Copy link

when we can expect a release with this feature?

@pawelprazak
Copy link

ignore me, I see it was just released in 0.11

keep up the good work, thanks!

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.

4 participants