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

Port mapping #165

Merged
merged 8 commits into from Jun 8, 2017

Conversation

Projects
None yet
4 participants
@jwhitcraft

This is to finish up the port mapping ticket in #135.

I added the help documents, and i can fix the other issue if @carlossg and provide guidance on how it should be fixed.

derfred and others added some commits Feb 17, 2017

Jon Whitcraft
Jon Whitcraft

@jwhitcraft jwhitcraft referenced this pull request Jun 8, 2017

Closed

Implement Port Mapping #135

Show outdated Hide outdated ...main/java/org/csanchez/jenkins/plugins/kubernetes/ContainerTemplate.java
public void setPorts(List<PortMapping> ports) {
if (ports != null) {
this.ports.clear();
this.ports.addAll(ports);

This comment has been minimized.

@carlossg

carlossg Jun 8, 2017

the way jenkins (de)serializes is a bit confusing. In upgrades this.ports will be null because the field is not in the xml config.
What you want is to guard against that, so just remove final (I don't think there's any reason to have it) and do this.ports=ports

@carlossg

carlossg Jun 8, 2017

the way jenkins (de)serializes is a bit confusing. In upgrades this.ports will be null because the field is not in the xml config.
What you want is to guard against that, so just remove final (I don't think there's any reason to have it) and do this.ports=ports

This comment has been minimized.

@jwhitcraft

jwhitcraft Jun 8, 2017

Sounds good, I'll update it in a few and push it up.

@jwhitcraft

jwhitcraft Jun 8, 2017

Sounds good, I'll update it in a few and push it up.

This comment has been minimized.

Jon Whitcraft
Show outdated Hide outdated ...main/java/org/csanchez/jenkins/plugins/kubernetes/ContainerTemplate.java
@@ -169,8 +169,7 @@ public void setLivenessProbe(ContainerLivenessProbe livenessProbe) {
@DataBoundSetter
public void setPorts(List<PortMapping> ports) {
if (ports != null) {
this.ports.clear();
this.ports.addAll(ports);
this.ports = ports;

This comment has been minimized.

@carlossg

carlossg Jun 8, 2017

no need to check for null now

public void setPorts(List<PortMapping> ports) {
  this.ports = ports;
}
@carlossg

carlossg Jun 8, 2017

no need to check for null now

public void setPorts(List<PortMapping> ports) {
  this.ports = ports;
}
Jon Whitcraft

@carlossg carlossg merged commit a667a9f into jenkinsci:master Jun 8, 2017

1 check passed

Jenkins This pull request looks good
Details
@carlossg

This comment has been minimized.

Show comment
Hide comment
@carlossg

carlossg Jun 8, 2017

I am merging it because it's been around for long, but it would be nice to have a pipeline test

carlossg commented Jun 8, 2017

I am merging it because it's been around for long, but it would be nice to have a pipeline test

@negz

This comment has been minimized.

Show comment
Hide comment
@negz

negz Jun 9, 2017

@carlossg Do you have a feeling of when the next release will be? No pressure - just wondering whether I should wait or build from master for now.

negz commented Jun 9, 2017

@carlossg Do you have a feeling of when the next release will be? No pressure - just wondering whether I should wait or build from master for now.

@carlossg

This comment has been minimized.

Show comment
Hide comment
@carlossg

carlossg Jun 9, 2017

no pressure taken ;)
I need testers for #157 to +1 it so I know it's not breaking everything

carlossg commented Jun 9, 2017

no pressure taken ;)
I need testers for #157 to +1 it so I know it's not breaking everything

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