-
Notifications
You must be signed in to change notification settings - Fork 331
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
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. |
ac7ac9d
to
7e710d2
Compare
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
f265b6f
to
6d8ba1a
Compare
b28e521
to
66ac257
Compare
✅ Deploy Preview for hypershift-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
d3f0bbd
to
c695ed8
Compare
/retest |
Seems like a flake but some jobs are passing now (new development) /retest-required |
/approve |
/retest |
62120ec
to
3daeed6
Compare
/retest |
/hold cancel |
/retest |
3daeed6
to
3e68ee3
Compare
/lgtm |
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]>
Signed-off-by: Steve Kuznetsov <[email protected]>
3e68ee3
to
ee7e223
Compare
/lgtm |
@stevekuznetsov: The following test 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. |
ba61c73
into
openshift:main
[ART PR BUILD NOTIFIER] This PR has been included in build ose-hypershift-container-v4.17.0-202406130611.p0.gba61c73.assembly.stream.el9 for distgit hypershift. |
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: