Skip to content
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

refactor cluster create command (frontend only) #10334

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Orzelius
Copy link
Contributor

@Orzelius Orzelius commented Feb 11, 2025

closes #10289
related: #10133, #10287

Changes

User facing

  • Add two sub-commands cluster create qemu and cluster create docker
    • The commands only have flags relevant to the applicable providers (cluster create still functions all the same)
  • Error if invalid provider's flags are passed (both new and old cluster create commands) (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.
  • Add (QEMU only) or (docker only) at the start of the flag description
  • In command description (e.g. cluster create -h) group flags by providers and then sort alphabetically.

See results to the command documentation in the reference docs

Internal

  • Split common code into the clustermaker package
  • Split qemu and docker code
  • Add basic unit tests for all options
  • Split common and provider specific cli options into separate structs and flag groups.

other

Notes 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


var testedFields = []string{}

func TestAllOptionFields(t *testing.T) {
Copy link
Contributor Author

@Orzelius Orzelius Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 <[email protected]>
@Orzelius
Copy link
Contributor Author

Update: rebased latest changes from main and removed the KVM change to reduce the scope of the pr.

@Orzelius
Copy link
Contributor Author

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:

  1. Split flags and options into separate provider specific structs
  2. Dependency inject provider and add tests
  3. Split out common logic
  4. Split qemu and docker logic
  5. Add separate create docker and create qemu commands

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

talosctl cluster create command refactor
1 participant