Reopen "get k8s config from requestConfig. and added test." #1692
Conversation
|
IIUC, this PR will allow to configure K8 in workflow definition ? |
|
@yoyama I'm sorry if it's not what I intended. I would like to add a parameter that allows setting using the parameter of "kubernetes" from the workflow. If this parameter is true, requestConfig is merged with systemConfig and used like PR. If false, only systemConfig is used. (As an image, the original createFromSystemConfig method) If this parameter is not set or defaults, it is treated as false. |
| if (extractedSystemConfig != null && extractedRequestConfig != null) { | ||
| config = extractedSystemConfig.merge(extractedRequestConfig); | ||
|
|
||
| } else if (extractedRequestConfig != null) { |
There was a problem hiding this comment.
May I ask you to change the format ?
}
else if (
| // from system config | ||
| return KubernetesClientConfig.createFromSystemConfig(name, systemConfig); | ||
|
|
||
| String clusterName = null; |
|
|
||
| // Create a config that merges RequestConfig with SystemConfig | ||
| final Config config; | ||
| if (extractedSystemConfig != null && extractedRequestConfig != null) { |
There was a problem hiding this comment.
If you set empty Config as default value in both extractedSystemConfig and extractedRequestConfig.
You can simply merge them without if ... else if ... ?
And at last check required keys are set.
| config = extractedSystemConfig; | ||
|
|
||
| } else { | ||
| throw new ConfigException("systemConfig and requestConfig does not exist"); |
There was a problem hiding this comment.
It might be better to improve the message to make users understand what is wrong.
|
@yoyama I don't know if this is intended by yoyama san, so please tell me your opinion. I fixed it based on some indications in the code. Please review. |
|
@hnarimiya Could you rebase your PR to the latest master and fix the CI failures ? |
hnarimiya
force-pushed
the
k8s-requestconfig
branch
from
0b6b32c to
7646259
Compare
3 participants
I rebased this PR(#1279) from master.
Please review.