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

cli: node groups #2152

Merged
merged 12 commits into from
Aug 4, 2023
Merged

cli: node groups #2152

merged 12 commits into from
Aug 4, 2023

Conversation

malt3
Copy link
Contributor

@malt3 malt3 commented Aug 2, 2023

Context

This is the second part of nodeGroup enablement. While previously, we modified the terraform templates to support nodeGroups, this PR exposes nodeGroups to the user via the constellation-conf.yaml.

As a side-effect, we deprecate the "--control-plane-nodes" and "--worker-nodes" flags from "constellation create".

Proposed change(s)

  • add nodeGroups to config
  • update config version to v4
  • deprecate old flags
  • remove deprecated flags from docs

Future changes (for other PRs)

  • add section in the docs to guide users on how to use node groups
  • config migration (v3 -> v4)

Checklist

  • Update docs
  • Add labels (e.g., for changelog category)
  • Is PR title adequate for changelog?
  • Link to Milestone
  • e2e test

@netlify

This comment was marked as off-topic.

@malt3 malt3 added the feature This introduces new functionality label Aug 2, 2023
@malt3 malt3 added this to the v2.10.0 milestone Aug 2, 2023
@malt3 malt3 marked this pull request as ready for review August 2, 2023 10:22
@katexochen katexochen removed their request for review August 2, 2023 10:46
@malt3 malt3 marked this pull request as draft August 2, 2023 12:03
@malt3 malt3 marked this pull request as ready for review August 2, 2023 12:15
@derpsteb derpsteb added the breaking change Change breaks existing API or configuration. label Aug 2, 2023
@malt3 malt3 force-pushed the feat/cli/node-groups branch 2 times, most recently from c2b1211 to 4847689 Compare August 2, 2023 14:18
dev-docs/howto/nfs.md Outdated Show resolved Hide resolved
internal/config/config.go Outdated Show resolved Hide resolved
internal/config/config_test.go Show resolved Hide resolved
@elchead
Copy link
Contributor

elchead commented Aug 3, 2023

Some doc comments:

  • how to migrate the config to NodeGroups.
  • describe what values can be set for each field of the NodeGroup, specifically Zone (link to CSP), InstanceType and StateDiskType.
  • explicitly state that you can configure multiple node groups of the same role to allow different node setups
  • state that at least a control_plane_default and worker_default nodeGroup are required (no renaming) since we validate the config, it seems easier to maintain only the code

@elchead
Copy link
Contributor

elchead commented Aug 3, 2023

like the config validation :)
For the regions / zones @thomasten said that we don't want to hardcode specific values in the validation, so that we can support new types without having to release a CLI. Is this different here?

internal/config/config.go Outdated Show resolved Hide resolved
internal/config/config.go Outdated Show resolved Hide resolved
@malt3 malt3 requested review from derpsteb and elchead August 3, 2023 16:00
internal/config/validation.go Outdated Show resolved Hide resolved
docs/docs/getting-started/first-steps.md Show resolved Hide resolved
docs/docs/getting-started/first-steps.md Show resolved Hide resolved
docs/docs/getting-started/first-steps-local.md Outdated Show resolved Hide resolved
docs/docs/workflows/create.md Outdated Show resolved Hide resolved
docs/docs/workflows/create.md Outdated Show resolved Hide resolved
docs/docs/workflows/troubleshooting.md Outdated Show resolved Hide resolved
@malt3 malt3 requested a review from thomasten August 4, 2023 07:38
@derpsteb derpsteb removed their request for review August 4, 2023 07:53
Copy link
Member

@derpsteb derpsteb left a comment

Choose a reason for hiding this comment

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

Can't remove myself from PR :(
Didn't look in depth.

@malt3 malt3 force-pushed the feat/cli/node-groups branch 3 times, most recently from 521a6d1 to b3435ee Compare August 4, 2023 09:51
@malt3 malt3 merged commit 7bfcb0b into main Aug 4, 2023
7 checks passed
@malt3 malt3 deleted the feat/cli/node-groups branch August 4, 2023 10:36
@elchead elchead mentioned this pull request Aug 8, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Change breaks existing API or configuration. feature This introduces new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants