-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
7c6a61c
to
4de5cf2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments, mostly about design around flags but it's probably also just fine how it currently is :)
apiServerFlags.add(key); | ||
} | ||
|
||
private void checkKeyPrefix(String key) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it'd be better to issue a warning and add the prefix automatically if it's missing to be more user-friendly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually yes, was thinking to ask for the flag without the flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a problem with the list of flags. I will merge this simple form, and we can do impovements in a subsequent PR.
private final List<String> apiServerFlags; | ||
|
||
KubeAPIServerConfig(String jenvtestDir, String apiServerVersion, boolean offlineMode, | ||
List<String> apiServerFlags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it could be useful to have a Flag
class that would encapsulate things instead of just relying on String pairs (similarly to what's done for micrometer tags)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh not sure, it is also some time not a pair, just a single flag name
"--disable-admission-plugins", "ServiceAccount", | ||
"--client-ca-file", certManager.getClientCertPath(), | ||
"--service-cluster-ip-range", "10.0.0.0/24", | ||
"--allow-privileged")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would one capture such a parameter if you're supposed to pass pairs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is also api for that. in the builder
No description provided.