8000 cli: node groups by malt3 · Pull Request #2152 · edgelesssys/constellation · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

cli: node groups #2152

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.

Sign up for GitHub

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

Jump to bottom
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 •
edited by laralaske
Loading

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.

Sign in to view
@malt3 malt3 added the feature This introduces new functionality label Aug 2, 2023
@malt3 malt3 force-pushed the feat/cli/node-groups branch from a8c814e to ac9b146 Compare August 2, 2023 09:19
@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
daniel-weisse
daniel-weisse reviewed Aug 2, 2023
View reviewed changes
@malt3 malt3 marked this pull request as draft August 2, 2023 12:03
@malt3 malt3 force-pushed the feat/cli/node-groups branch from ac9b146 to e5f7b90 Compare August 2, 2023 12:14
@malt3 malt3 marked this pull request as ready for review August 2, 2023 12:15
@malt3 malt3 force-pushed the feat/cli/node-groups branch from c458093 to de43470 Compare August 2, 2023 13:08
@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
elchead
elchead reviewed Aug 3, 2023
View reviewed changes
daniel-weisse
daniel-weisse approved these changes Aug 3, 2023
View reviewed changes
@elchead
Copy link
Contributor
elchead commented Aug 3, 2023 •
edited
Loading

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
elchead reviewed Aug 3, 2023
View reviewed changes
derpsteb
derpsteb reviewed Aug 3, 2023
View reviewed changes
10000
elchead
elchead reviewed Aug 3, 2023
View reviewed changes
elchead
elchead reviewed Aug 3, 2023
View reviewed changes
@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?

elchead
elchead reviewed Aug 3, 2023
View reviewed changes
elchead
elchead reviewed Aug 3, 2023
View reviewed changes
@malt3 malt3 requested review from derpsteb and elchead August 3, 2023 16:00
thomasten
thomasten requested changes Aug 3, 2023
View reviewed changes
@malt3 malt3 requested a review from thomasten August 4, 2023 07:38
@derpsteb derpsteb removed their request for review August 4, 2023 07:53
derpsteb
derpsteb approved these changes Aug 4, 2023
View reviewed changes
Copy link
Contributor
@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 force-pushed the feat/cli/node-groups branch from b3435ee to 24584f9 Compare August 4, 2023 10:06
@malt3 malt3 merged commit 7bfcb0b into main Aug 4, 2023
@malt3 malt3 deleted the feat/cli/node-groups branch August 4, 2023 10:36
This was referenced Aug 4, 2023
Merged
Merged
@elchead elchead mentioned this pull request Aug 8, 2023
Merged
4 tasks
@3u13r 3u13r mentioned this pull request Sep 26, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derpsteb derpsteb derpsteb approved these changes

@elchead elchead elchead approved these changes

@thomasten thomasten thomasten approved these changes

@daniel-weisse daniel-weisse daniel-weisse approved these changes

Assignees
No one assigned
Labels
breaking change Change breaks existing API or configuration. feature This introduces new functionality
Projects
None yet
Milestone
v2.10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@malt3 @elchead @derpsteb @thomasten @daniel-weisse
0