refactor cluster create command (frontend only) #10334
Conversation
|
|
||
| var testedFields = []string{} | ||
|
|
||
| func TestAllOptionFields(t *testing.T) { |
There was a problem hiding this comment.
Test that there are tests for all the common fields.
Please let me know if this is inappropriate or if there's a less hacky way to achieve the same result or if I should just change it to good ol' fashioned tests
Orzelius
force-pushed
the
create-refactor-2
branch
from
cc4b521 to
ce77931
Compare
|
Update: rebased latest changes from main and removed the KVM change to reduce the scope of the pr. |
|
If this is too big of a change to confidentially merge all at once please let me know. It would take some extra work, but I could split it into smaller, easier to review pieces. For example as follows:
This would result in a easier to review diffs where possible mistakes are easier to spot. If you generally disagree with the approach or implementation on broader terms please describe a more suitable solution in the #10289 issue. |
|
This PR is stale because it has been open 45 days with no activity. |
Orzelius
force-pushed
the
create-refactor-2
branch
from
ce77931 to
5627014
Compare
|
rebased latest changes |
User facing changes: * Add two sub-commands: "cluster create qemu" and "cluster create docker" The commands only have flags relevant to the applicable providers * Error if invalid provider's flags are passed * Add (QEMU only) or (docker only) at the start of the flag description * Group flags by providers and then sort alphabetically. Internal: * Split common cluster creation logic into a ClusterMaker abstraction * Split creation logic of cluster and qemu clusters * Add basic unit tests for all the options unrelated * Use default kubeconfig instead of kubeconfig.SinglePath() Signed-off-by: Orzelius <33936483+Orzelius@users.noreply.github.com>
Orzelius
force-pushed
the
create-refactor-2
branch
from
5627014 to
2a5406d
Compare
* recent rebased changes changed some behaviour and the tests also needed updating * do some slight code tidying * add some more comments Signed-off-by: Orzelius <33936483+Orzelius@users.noreply.github.com>
closes #10289
related: #10133, #10287
Changes
User facing
cluster create qemuandcluster create dockercluster createstill functions all the same)cluster createcommands) (previously not all the flags were checked). This is technically a breaking change. Please let me know if it should be a warning at first.(QEMU only)or(docker only)at the start of the flag descriptioncluster create -h) group flags by providers and then sort alphabetically.See results to the command documentation in the reference docs
Internal
clustermakerpackageother
kubeconfig.SinglePath()which errors if multiple configs are definedNotes to reviewers
This might look like a massive change, but it's mostly just moving existing logic around with added tests. I've failed to get the integration tests running locally, so I'd love to see the results of a ci run and will fix any issues if they come up. As I can't perform thorough qemu testing on my mac machine I'm fairly certain some bugs remain, but they should be easy to fix once found thanks to the testable logic and clear separation of concerns.
Please let me know if I've missed something or if you have any ideas <3