[JENKINS-71959] Rename blank cloud names to be non-blank #1010
Conversation
jenkinsci/jenkins#7658 reworks cloud management to use multiple pages instead of a single large page. That rework also requires that a cloud may not be named with an empty string. The docker plugin specifically tested that a cloud was allowed to have an empty string as its name, but performed no other operations with a cloud that is named with an empty string. Cloud named with an empty string may have worked prior to Jenkins 2.403, but they would have been complicated to manage, especially in a controller with multiple clouds defined. A cloud named with an empty string is much more difficult to manage. It is better to mention in the changelog that clouds are no longer allowed to have an empty name rather than wrestling with all the ways that an empty name will break cloud administration. https://issues.jenkins.io/browse/JENKINS-70729 is the enhancement request that implements the change. https://issues.jenkins.io/browse/JENKINS-71825 is the bug report for the automated test failure that started this investigation.
MarkEWaite
changed the title
Do not break existing usage of blank names, but warn in the log file that blank names are no longer supported after Jenkins 2.402. The Jenkins 2.421 cloud creation UI will not allow a blank name, but earlier versions may have allowed it. Assert the log message for blank or name cloud name
MarkEWaite
force-pushed
the
fix-test-with-newer-jenkins
branch
from
36aba28
to
1416f89
Compare
MarkEWaite
changed the title
MarkEWaite
changed the title
|
@gounthar and @Vlatombe I am sure there is a better way to handle this than telling the user to stop Jenkins and edit the name attribute in the conifig.xml file section for the docker plugin. However, I don't see it. There is a readResolve() method in DockerfCloud but I don't see how it can adjust the name attribute and have it applied correctly. Any suggestions? I like the new cloud management UI very much but would like to make the transition smooth for users of the Docker cloud plugin. If they made the mistake of naming their docker cloud with an empty string, they won't be able to edit from the web page once they are using Jenkins 2.403 or newer. |
| @@ -727,6 +730,10 @@ protected Object readResolve() { | |||
| dockerApi = new DockerAPI(dockerHost, connectTimeout, readTimeout, version, dockerHostname); | |||
| } | |||
|
|
|||
| if (name == null || name.isBlank()) { | |||
| LOGGER.warn("Docker cloud requires a non-blank name after Jenkins 2.402"); | |||
There was a problem hiding this comment.
| LOGGER.warn("Docker cloud requires a non-blank name after Jenkins 2.402"); | |
| LOGGER.warn("Docker cloud requires a non-blank name after Jenkins 2.402"); | |
| return new DockerCloud("docker-" + UUID.randomUUID().toString().substring(0, 8), this); |
And implement a copy constructor like this (this is what is used in kubernetes-plugin)
public DockerCloud(@NonNull String name, @NonNull DockerCloud source) {
super(name);
XStream2 xs = new XStream2();
xs.omitField(Cloud.class, "name");
xs.unmarshal(XStream2.getDefaultDriver().createReader(new StringReader(xs.toXML(source))), this);
}
Probably requires saving configuration afterwards (or switch uuid to a digest based on current content) to avoid getting a different name on each instance start.
There was a problem hiding this comment.
Thanks very much @Vlatombe !
The XStream2 call caused complaints from Jenkins core that it was trying to marshal unsafe entities, so I switched to a simple assignment in 57d3010 . I'll run some interactive tests to confirm that change is behaving as expected. Let me know if you detect errors in 57d3010
There was a problem hiding this comment.
Simple assignment works too, but can be source of maintenance issues because you need to remember that it needs to be updated every time a field is added.
Jenkins 2.403 improves the cloud management user interface. Each cloud is now managed from a separate page. That separate page requires a non-blank name for the cloud. Jenkins 2.402 and later allowed a blank name. When a blank name is detected, convert it to a non-blank name based on the hashCode of the DockerAPI and templates.
MarkEWaite
changed the title
MarkEWaite
changed the title
| @@ -731,7 +738,9 @@ protected Object readResolve() { | |||
| } | |||
|
|
|||
| if (name == null || name.isBlank()) { | |||
| LOGGER.warn("Docker cloud requires a non-blank name after Jenkins 2.402"); | |||
| String newName = "docker-cloud-" + Integer.toHexString(hashCode()); | |||
There was a problem hiding this comment.
hashCode() is not collision-free so could cause duplicate names.
There was a problem hiding this comment.
I confirmed that collisions happen when the hashCode values of two clouds are the same and the cloud name of both clouds is blank. Since the hashCode includes the DockerAPI object and the list of templates, I think it will be very rare that a Jenkins controller has more than one docker cloud definition that have the same values for DockerAPI and template list and use blank names for the cloud name.
I think it is better to risk a collision and prefer a stable transformation from blank name to generated name. If I remove the risk of collision, it seems like I am accepting that the transformation from blank name to generated name will choose a unique name each time it is run.
Does that seem like an acceptable risk to you or should I accept that the transformation will be different each time it is applied to a docker cloud that has a blank name?
There was a problem hiding this comment.
Worse thing that will happen is you will see the same entry twice in the cloud list, causing a weird UX. Clicking on one of them will show you the first entry appearing in the underlying persisted list.
I think it can still be renamed to a different name by the user to clear the ambiguity.
However until the rename happens, the second cloud with the same name won't be editable.
There was a problem hiding this comment.
When I did the rename in my test of editing the first in the list, it showed the new name for both in the web page. However, I didn't spend a lot of time exploring beyond that.
I think this is ready to merge. Interactive testing is complete. The pull request description includes details of that testing. Anything else that you feel should be changed before it is merged and released?
[JENKINS-71959] Rename blank cloud names to be non-blank
jenkinsci/jenkins#7658 reworks cloud management to use multiple pages instead of a single large page. That rework also requires that a cloud may not be named with an empty string.
The docker plugin previously tested that a cloud was allowed to have an empty string as its name, but performed no other operations with a cloud named with an empty string. Clouds named with an empty string may have worked prior to Jenkins 2.403, but they would have been complicated to manage, especially in a controller with multiple clouds defined.
A cloud named with an empty string is much more difficult to manage. It is better to mention in the changelog that clouds are no longer allowed to have an empty name rather than wrestling with all the ways that an empty name will break cloud administration.
Testing done
Confirmed that a Docker cloud named with an empty string can be edited in 2.401.3 with no apparent loss of functionality and when that cloud with the empty named string is loaded into the plugin changes with this pull request, it is assigned a name
docker-cloud-[hashCode]. That cloud can be edited in Jenkins 2.414.1 without any issue.Discovered that a Docker cloud name uniqueness check is not performed. I was able to create two distinct Docker clouds with the same name. Editing the Docker clouds that shared the same name resulted in erratic behavior. I could delete one of the two clouds that used the duplicate name, but immediately after that the other cloud with the same name would fail some operations. Handling duplicate Docker cloud names is out of scope for this pull request. Another pull request can be filed to handle that case.
Confirmed that Jenkins 2.421 automated tests fail as reported in JENKINS-71825 without this change.
Confirmed that Jenkins 2.421 automated tests pass with this change.
Confirmed that Jenkins 2.421 already rejects an empty cloud name from the cloud creation page, whether or not this change is implemented.
Confirmed that Jenkins 2.421 accepts an empty cloud name from an earlier version but then rejects all attempts to use the cloud configuration page to modify the definition. I modified the XML file that configures the cloud so that the cloud name was an empty string to perform that interactive test.
Submitter checklist