Fix #2399: Cannot change the type of the Service from ClusterIP to ExternalName #2472
Conversation
rohanKanojia
changed the title
rohanKanojia
force-pushed
the
pr/issue2399
branch
from
6f8cc09 to
87d288a
Compare
manusa
force-pushed
the
pr/issue2399
branch
from
87d288a to
03f942a
Compare
…rIP to ExternalName
manusa
force-pushed
the
pr/issue2399
branch
from
03f942a to
44ee47c
Compare
|
SonarCloud Quality Gate failed.
|
|
[merge] |
| // When | ||
| svc.getSpec().setType("ExternalName"); | ||
| svc.getSpec().setExternalName("my.database.example.com"); | ||
| svc.getSpec().setClusterIP(""); |
There was a problem hiding this comment.
No assertion for ClusterIP ?
There was a problem hiding this comment.
Services of type ExternalName don't need ClusterIP, API Servers enforces the ClusterIP field to be empty when editing the 'type' field from ClusterIP to ExternalName
| } | ||
|
|
||
| @Test | ||
| public void testClusterIPCreateOrReplace() { |
There was a problem hiding this comment.
The name is a bit misleading.. can we change it to testClusterIpServiceCreateOrReplace ?
There was a problem hiding this comment.
Yes, I'll create a follow up PR with your suggested changes
| } | ||
|
|
||
| @Test | ||
| public void testNodePortCreateOrReplace() { |
There was a problem hiding this comment.
same thing with the name
| // Then | ||
| assertNotNull(clusterIPSvc); | ||
| assertEquals("NodePort", clusterIPSvc.getSpec().getType()); | ||
| assertEquals(81, clusterIPSvc.getSpec().getPorts().get(0).getTargetPort().getIntVal().intValue()); |
There was a problem hiding this comment.
We should also check for the NodePort value created.
There was a problem hiding this comment.
I added this assertion to check whether what I modified in When phase is actually reflected or not. But yes, I can try doing something with NodePort too
| } | ||
|
|
||
| @Test | ||
| public void testLoadBalancerCreateOrReplace() { |
| } | ||
|
|
||
| @Test | ||
| public void testExternalNameCreateOrReplace() { |
This PR addresses review comments added in fabric8io#2472 which were added after PR actually got merged
| } catch (Exception e) { | ||
| throw KubernetesClientException.launderThrowable(forOperationType("patch"), e); | ||
| } | ||
| try { |
There was a problem hiding this comment.
Shouldn't the patch also work with ExternalName?
There was a problem hiding this comment.
I think you're right. Could you please create an issue for this?
Description
Fix #2399
Removes overridden replace() method in
ServiceOperationsImplType of change
test, version modification, documentation, etc.)
Checklist