Require Jenkins 2.414.3 or newer #1016
Conversation
car-roll
force-pushed
the
dedicated-cloud-rename-page
branch
from
21dad94
to
e88d5df
Compare
| @@ -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 | |||
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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.
| @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()); |
There was a problem hiding this comment.
@MarkEWaite I kept the CopyConstructor test since it covers backwards compatibility, but I had to modify it to allow an "empty" named DockerCloud
There was a problem hiding this comment.
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.
car-roll
changed the title
|
Note: jenkinsci/jenkins#8310 has now been closed, but i think the test changes for this PR will be needed whenever the jenkins version is bumped past 2.403. This is when cloud name is not allowed to be empty |
car-roll
changed the title
MarkEWaite
changed the title
|
@car-roll I assume this does not require a new release but can be included in the next release whenever that happens. Is that correct? |
|
@MarkEWaite yes, that's correct. It was more about just not having someone else going through resolving those same issues I went through if I closed this PR completely |
Require Jenkins 2.414.3 or newer
Also removes tests that relied on an empty cloud name being allowed.
was originally downstream of jenkinsci/jenkins#8310 but that has now been abandoned
since Jenkins 2.403, it is no longer possible to create a cloud with an empty name. This fixes tests that create empty clouds.
Testing done
N/A - changes only in test automation are well covered by ci.jenkins.io
Submitter checklist