add kube helper #71
add kube helper #71
Conversation
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
MichaelRoeder
left a comment
MichaelRoeder
left a comment
There was a problem hiding this comment.
The class looks good but I would suggest to have a more "neutral" usage of terms, i.e., we are not looking for pod names and this is a class that should not only work with kubernetes 😉
| } catch (UnknownHostException e) { | ||
| // Handle the exception, e.g., log an error or return a default value. | ||
| LOGGER.error("Error getting pod IP: " + e.getMessage()); | ||
| return "unknown"; // Or throw a RuntimeException, etc. |
There was a problem hiding this comment.
Does it make sense to not fail and continue the experiment in this case? While some containers might not need themselves to be addressable, we currently don't have a way to tell this in advance.
There was a problem hiding this comment.
you are right, in this case, it is better to fail (now it will throw an exception in getDnsFriendlyIP ), but anyway the unknown is not reachable and the module will fail in the next step.
There was a problem hiding this comment.
Do we really need "unknown", here? Wouldn't returning null make more sense? This would also fit very well to the if statement in the other method, that checks for null anyway...
I see that this is a Kubernetes-specific helper, similar to how the Docker version exists in the core. Given that, I’m not sure I fully understand your point about making it work beyond Kubernetes. Right now, the client (any module using this) needs to determine whether it's running in a Kubernetes environment. If it is, it would use this helper to get the host. Do you have a different approach in mind? |
|
That is a good question 🙂 From my perspective, we have a program and it simply wants to get the address at which it can be reached. It calls this helper class and the class will give it the information it needs. Note the following implications:
I even would raise the question whether the "namespace" string is really necessary... shouldn't this be something that the helper class can derive, somehow? |
- Introduced dnsHelper to be used by modules. - Ensures backward compatibility. - No changes made to the DockerHelper class.
…is kubernetes or docker or anything in future :D
| } catch (UnknownHostException e) { | ||
| // Handle the exception, e.g., log an error or return a default value. | ||
| LOGGER.error("Error getting pod IP: " + e.getMessage()); | ||
| return "unknown"; // Or throw a RuntimeException, etc. |
There was a problem hiding this comment.
Do we really need "unknown", here? Wouldn't returning null make more sense? This would also fit very well to the if statement in the other method, that checks for null anyway...
No description provided.