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

Require Jenkins 2.414.3 or newer #1016

Merged
merged 6 commits into from
Nov 17, 2023
Merged

Require Jenkins 2.414.3 or newer #1016

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e88d5df
Refactor to support separate cloud rename page
car-roll Sep 12, 2023
0103d7e
spotless
car-roll Sep 13, 2023
e84b15c
update jenkins incremental
car-roll Sep 14, 2023
33e6941
Merge branch 'master' into dedicated-cloud-rename-page
car-roll Nov 17, 2023
8cf32d3
add name back in config.jelly
car-roll Nov 17, 2023
ea7f0e9
fix jenkins version
car-roll Nov 17, 2023
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
5 changes: 3 additions & 2 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@
<properties>
<revision>1.6</revision>
<changelist>-SNAPSHOT</changelist>
<jenkins.version>2.387.3</jenkins.version>
<jenkins.version>2.423-rc34199.8c3d6c65b_758</jenkins.version>
<!-- TODO: https://github.com/jenkinsci/jenkins/pull/8310 -->
<gitHubRepo>jenkinsci/docker-plugin</gitHubRepo>
<!-- Our unit-tests that talk to a real docker deamon aren't very stable -->
<surefire.rerunFailingTestsCount>3</surefire.rerunFailingTestsCount>
Expand All @@ -76,7 +77,7 @@
<dependencies>
<dependency>
<groupId>io.jenkins.tools.bom</groupId>
<artifactId>bom-2.387.x</artifactId>
<artifactId>bom-2.414.x</artifactId>
<version>2401.v7a_d68f8d0b_09</version>
<type>pom</type>
<scope>import</scope>
Expand Down
4 changes: 0 additions & 4 deletions src/main/resources/com/nirima/jenkins/plugins/docker/DockerCloud/config.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form" xmlns:c="/lib/credentials" >

<f:entry title="${%Name}" field="name">
<f:textbox default="docker"/>
</f:entry>

<f:advanced title="${%Docker Cloud details}" align="left">

<f:property field="dockerApi"/>
Expand Down
24 changes: 2 additions & 22 deletions src/test/java/com/nirima/jenkins/plugins/docker/DockerCloudTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,34 +58,14 @@ public void testConstructor_0_10_2() {

private static final String LOG_MESSAGE = "Docker cloud requires a non-blank name after Jenkins 2.402";

@Issue("JENKINS-70729") // Warn if cloud name is empty
Copy link
Contributor Author

@car-roll car-roll Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@MarkEWaite I removed these two tests because the base class (Cloud) now throws an IllegalArgumentException that does not allow the creation of DockerCloud. The warning log message will now never be generated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @car-roll ! Has the base class been extended to handle upgrades on systems that already have a Docker cloud named with an empty string? If not, that seems like it will be needed in order to not break upgrades.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the base class has not been extended to support empty string Cloud names, but support is still there as I did not touch the readResolve in DockerCloud.

Just to clarify what I meant, I believe the code that these two tests are testing (testConstructorWithEmptyName and testConstructorWithNullName) are testing the constructor itself

docker-plugin/src/main/java/com/nirima/jenkins/plugins/docker/DockerCloud.java

Line 136 in ccec39c

public DockerCloud(String name, DockerAPI dockerApi, List<DockerTemplate> templates) {

However, with this new jenkins version, we are now failing on the super(name) call, so that log message will now never be called. Test-wise, I do not think I can reproduce this scenario with the new Jenkins version.

The only time now we will have to deal with a null name constructor should be in existing cloud names, which is tested in testCopyConstructor.

@Test
public void testConstructorWithEmptyName() {
lr.record(DockerCloud.class.getName(), Level.ALL).capture(16);
DockerCloud cloud =
new DockerCloud("", new DockerAPI(new DockerServerEndpoint("uri", "credentialsId")), List.of());
Assert.assertEquals(cloud.getDisplayName(), "");
MatcherAssert.assertThat(lr.getMessages(), IsIterableContaining.hasItem(LOG_MESSAGE));
}

@Issue("JENKINS-70729") // Warn if cloud name is null
@Test
public void testConstructorWithNullName() {
lr.record(DockerCloud.class.getName(), Level.ALL).capture(16);
DockerCloud cloud =
new DockerCloud(null, new DockerAPI(new DockerServerEndpoint("uri", "credentialsId")), List.of());
Assert.assertEquals(cloud.getDisplayName(), null);
MatcherAssert.assertThat(lr.getMessages(), IsIterableContaining.hasItem(LOG_MESSAGE));
}

@Issue("JENKINS-70729") // Handle null or empty cloud name
@Test
public void testCopyConstructor() {
lr.record(DockerCloud.class.getName(), Level.ALL).capture(16);
DockerCloud cloud =
new DockerCloud(null, new DockerAPI(new DockerServerEndpoint("uri", "credentialsId")), List.of());
new DockerCloud("tmp", new DockerAPI(new DockerServerEndpoint("uri", "credentialsId")), List.of());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MarkEWaite I kept the CopyConstructor test since it covers backwards compatibility, but I had to modify it to allow an "empty" named DockerCloud

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @car-roll . That makes sense to me. Assigning directly to the name attribute is a nice technique. I would not have thought of that.

cloud.name = null;
Assert.assertEquals(cloud.getDisplayName(), null);
MatcherAssert.assertThat(lr.getMessages(), IsIterableContaining.hasItem(LOG_MESSAGE));
String newName = "docker-cloud-" + Integer.toHexString(cloud.hashCode());
DockerCloud copy = new DockerCloud(newName, cloud);
Assert.assertEquals(cloud.getDockerApi(), copy.getDockerApi());
Expand Down