Skip to content

Conversation

Orzelius
Copy link
Member

@Orzelius Orzelius commented Sep 20, 2025

Refactor the cluster creation code (once more) to assure easy maintainability and testability

  • Create the following packages under /create:
    • flags containing the pflag.Value implementations
    • clusterops containing the options for cluster creation
      • configmaker containing the logic that aids in creation of talos and provision configuration for cluster creation
        • internal makers and siderolinkbuilder packages that hide the internal logic that configmaker uses
  • Remove code duplication of default values. Now all default values come from the clusterops Get functions.
  • Add unit tests for flag implementations.
  • Add tests that compare machine configs generated for cluster create to default configs.
    These tests also functions as snapshot tests and will asure no undesired changes pass through in the future unnoticed.

Potential further improvements that could be made if judged beneficial enough:

  • Split the dev and user-facing qemu creators. This would isolate our development related changes such that they don't cause issues with the user-facing dev command. If implemented, the split should be made such that the dev-qemu-maker extends the user-facing-qemu-maker, keeping us dogfooding the logic.
  • Remove the purely CLI related options from clusterops (stuff like StateDir, CniBinPath, TalosconfigDestination etc)

part of #11404

@github-project-automation github-project-automation bot moved this to To Do in Planning Sep 20, 2025
@Orzelius Orzelius force-pushed the create-cluster-refactor branch 6 times, most recently from 02abc75 to b9bc716 Compare September 22, 2025 12:00
@Orzelius Orzelius changed the title wip: cluster create refactor cluster create refactor Sep 22, 2025
@Orzelius Orzelius marked this pull request as ready for review September 22, 2025 12:14
@talos-bot talos-bot moved this from To Do to In Review in Planning Sep 22, 2025
@Orzelius Orzelius force-pushed the create-cluster-refactor branch 2 times, most recently from 1743274 to b9bc716 Compare September 24, 2025 06:25
@Orzelius
Copy link
Member Author

Orzelius commented Sep 24, 2025

@frezbo Thank you for the feedback! I've implemented the changes in a6bc0fb so it's easier to review. I will squash it before merging. I also renamed the methods and fixed a bug where the version contract was initialized too late causing latest configs to be generated even for older talos version.

Copy link
Member

@smira smira left a comment

Choose a reason for hiding this comment

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

👍 as long as it works

@github-project-automation github-project-automation bot moved this from In Review to Approved in Planning Sep 26, 2025
* Create the following packages under `/create`:
	- `flags` containing the pflag.Value implementations
	- `clusterops` containing the options for cluster creation
		- `configmaker` containing the logic that aids in creation of talos and provision configuration for cluster creation
			- internal `makers` and `siderolinkbuilder` packages that hide the internal logic that configmaker uses
* Remove code duplication of default values. Now all default values come from the clusterops Get functions.
* Add unit tests for flag implementations.
* Add tests that compare machine configs generated for cluster create to default configs.
	These tests also functions as snapshot tests and will asure no undesired changes pass through in the future unnoticed.

Signed-off-by: Orzelius <33936483+Orzelius@users.noreply.github.com>
@Orzelius Orzelius force-pushed the create-cluster-refactor branch from a6bc0fb to 65a6609 Compare September 29, 2025 13:21
@Orzelius
Copy link
Member Author

/m

@talos-bot talos-bot merged commit 65a6609 into siderolabs:main Sep 29, 2025
62 checks passed
@github-project-automation github-project-automation bot moved this from Approved to Done in Planning Sep 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants