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

Added tool installations to the pod template. #85

Merged
merged 6 commits into from Nov 10, 2016

Conversation

Projects
None yet
2 participants
@iwarapter

iwarapter commented Oct 31, 2016

No description provided.

@carlossg

Needs the help text to go with the field, and some docs on how to use it

@iwarapter

This comment has been minimized.

Show comment
Hide comment
@iwarapter

iwarapter Nov 8, 2016

The ToolLocationNodeProperty come with descriptors/help for the tool installations section is this ok or do you think we need more?

Do you have any preference where the README is updated? I was going to add a short section under Modify default request/limits (Kubernetes Resource API) with a simple screenshot and short description.

iwarapter commented Nov 8, 2016

The ToolLocationNodeProperty come with descriptors/help for the tool installations section is this ok or do you think we need more?

Do you have any preference where the README is updated? I was going to add a short section under Modify default request/limits (Kubernetes Resource API) with a simple screenshot and short description.

@carlossg

This comment has been minimized.

Show comment
Hide comment
@carlossg

carlossg Nov 8, 2016

ah, sorry, I'm not familiar with the tool installers. I just gave it a try and looks good in the UI, so I think it is good to go

carlossg commented Nov 8, 2016

ah, sorry, I'm not familiar with the tool installers. I just gave it a try and looks good in the UI, so I think it is good to go

Show outdated Hide outdated src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java
}
public List<ToolLocationNodeProperty> getNodeProperties(){
return nodeProperties;

This comment has been minimized.

@carlossg

carlossg Nov 8, 2016

should return emptyList if nodeProperties is null ?

please set the @Nonnull annotation if that's the case

@carlossg

carlossg Nov 8, 2016

should return emptyList if nodeProperties is null ?

please set the @Nonnull annotation if that's the case

Show outdated Hide outdated src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java
@@ -73,6 +74,8 @@
private final List<PodImagePullSecret> imagePullSecrets = new ArrayList<PodImagePullSecret>();
private final List<ToolLocationNodeProperty> nodeProperties = Collections.emptyList();

This comment has been minimized.

@carlossg

carlossg Nov 9, 2016

I don't think this will work in an upgrade, nodeProperties will be null.
Either implement readResolve to set it to emptyList if null https://github.com/jenkinsci/kubernetes-plugin/blob/master/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java#L348
or check in getNodeProperties

@carlossg

carlossg Nov 9, 2016

I don't think this will work in an upgrade, nodeProperties will be null.
Either implement readResolve to set it to emptyList if null https://github.com/jenkinsci/kubernetes-plugin/blob/master/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java#L348
or check in getNodeProperties

Show outdated Hide outdated src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java
@@ -1,5 +1,6 @@
package org.csanchez.jenkins.plugins.kubernetes;
import edu.umd.cs.findbugs.annotations.NonNull;

This comment has been minimized.

@carlossg

carlossg Nov 10, 2016

sorry, should be javax.annotation.Nonnull

@carlossg

carlossg Nov 10, 2016

sorry, should be javax.annotation.Nonnull

@carlossg carlossg merged commit 4200b2b into jenkinsci:master Nov 10, 2016

1 check passed

Jenkins This pull request looks good
Details
@carlossg

This comment has been minimized.

Show comment
Hide comment
@carlossg

carlossg commented Nov 10, 2016

thanks!

@iwarapter

This comment has been minimized.

Show comment
Hide comment
@iwarapter

iwarapter Nov 10, 2016

yw, any idea when the next release is likely to be?

iwarapter commented Nov 10, 2016

yw, any idea when the next release is likely to be?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment