-
Notifications
You must be signed in to change notification settings - Fork 296
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
HOSTEDCP-1542: cmd/cluster: refactor to remove example fixtures #4018
base: main
Are you sure you want to change the base?
HOSTEDCP-1542: cmd/cluster: refactor to remove example fixtures #4018
Conversation
@stevekuznetsov: This pull request references HOSTEDCP-1542 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: stevekuznetsov The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ac7ac9d
to
7e710d2
Compare
fd587fa
to
2a2731c
Compare
The goal of this refactor is to reduce the complexity in the command-line tooling. Overall, the changes here remove duplicative structures that copied data around and co-locate the logic with the data, instead of first aggregating all data into one uber-structure and then conditionally acting on that structure. These refactors have a number of benefits: - locality of behavior: in the past, it was very difficult if not impossible to determine where a value was used, as it would be bound to a flag in one package, copied around between container structs a couple of times, then have some generic logic act on the presence or absence of the value to e.g. change a field on the HostedCluster. Simply reading the generic logic was often not enough to understand what was going on, as many of the conditional branches in the example fixture code could only ever trigger for one specific platform, and you'd never know unless you traced how the example options uber-struct had its fields set in every provider. - clear go-to-definition: as a knock-on effect of the above, now there's *one* structure that holds a command-line flag and it's trivial to use the LSP when determining where that flag is used and how - composability: as exemplified in the KubeVirt NodePool code, we are able to compose commands as necessary. When commands re-use the same arguments with the same flags and the same validation logic, there's no need to copy things around and re-implement anything; by localizing flag binding, validation and option completion, we gain small, composable parts that we can use to build larger commands with Signed-off-by: Steve Kuznetsov <[email protected]>
We only bind flags in one routine now; breaking out explicitly the set of flags that should only be exposed to developers in the `hypershift` CLI. The net effect of this change is to expose `--base-domain-prefix` and `--external-dns-domain` to users of `hcp`. This change also shows how to change the defaults in an option set for a command - the `hcp create cluster` command has a unique default for the control plane availability policy, and its now evident that this is the case since it has to be done explicitly after building the default set of options. Signed-off-by: Steve Kuznetsov <[email protected]>
Signed-off-by: Steve Kuznetsov <[email protected]>
9725406
to
f265b6f
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.
kubevirt parts look accurate
Signed-off-by: Steve Kuznetsov <[email protected]>
f265b6f
to
6d8ba1a
Compare
When the test time out in Prow, the test infra sends us a SIGINT and gives us a grace period to handle it, do cleanup, etc. There's no need for us to do our own timeout, which causes panics and leads to corrupt jUnit outpout. Signed-off-by: Steve Kuznetsov <[email protected]>
@stevekuznetsov: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
The goal of this refactor is to reduce the complexity in the command-line tooling. Overall, the changes here remove duplicative structures that copied data around and co-locate the logic with the data, instead of first aggregating all data into one uber-structure and then conditionally acting on that structure. These refactors have a number of benefits: