-
Notifications
You must be signed in to change notification settings - Fork 2k
Feat: Kubectl get implementation #1251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
return !StringUtils.isEmpty(namespace); | ||
} | ||
|
||
public class KubectlGetSingle implements Kubectl.Executable<ApiType> { |
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.
This is going to get messy/confusing to people because:
Kubectl.get().namespace(foo).name(bar).execute()
will work, but Kubectl.get().name(bar).namespace(foo)
won't.
I think we should either get rid of this class and merge the execute methods, or make this class extend from KubectlGet
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.
Kubectl.get().namespace(foo).name(bar).execute() will work, but Kubectl.get().name(bar).namespace(foo) won't.
yeah i agree this can be confusing to the users. while the point here is that we're returning single object upon specifying resource name, and return a list of objects if the name absent. java doesn't support multi-return or overriding return type for the execute method so merging or overriding the execute methods won't work..
in a word, if we're to make both namespace(foo).name(bar)
and name(bar).namespace(foo)
work, the execute method will have to return a list of object even when getting one object, which can be even more confusing i suppose?
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.
Hrm, why not just have, and then you can do:
return new KubectlGetSingle().namespace(namespace).name(name)
in the function that adds the name, then it will work either way.
public KubectlGetSingle<ApiType> extends ResourceBuilder<...>, implements Kubectl.Executable<ApiType>
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.
done
Generally looks good, one comment about the interface. |
a427040
to
4af3e03
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, yue9944882 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ideally i think this can be a replacement for GenericKubernetesApi