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: enable constellation apply to create new clusters #2549

Merged
merged 22 commits into from
Nov 20, 2023

Conversation

daniel-weisse
Copy link
Member

@daniel-weisse daniel-weisse commented Nov 2, 2023

Context

The constellation apply command should be the one command to manage a Constellation cluster.
This PR brings the functionality of another command (constellation create) to apply.

Proposed change(s)

  • Use the logic of the constellation apply for creating new cluster, replacing constellation create

Additional info

Checklist

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

Copy link

netlify bot commented Nov 2, 2023

Deploy Preview for constellation-docs ready!

Name Link
🔨 Latest commit 15de42b
🔍 Latest deploy log https://app.netlify.com/sites/constellation-docs/deploys/655b25634f2bec00088818a0
😎 Deploy Preview https://deploy-preview-2549--constellation-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@daniel-weisse daniel-weisse marked this pull request as ready for review November 2, 2023 10:27
@daniel-weisse daniel-weisse added the feature This introduces new functionality label Nov 2, 2023
@daniel-weisse daniel-weisse changed the title cli: move create logic to constellation apply cli: enable constellation apply to create new clusters Nov 2, 2023
@daniel-weisse daniel-weisse added this to the v2.13.0 milestone Nov 2, 2023
@daniel-weisse daniel-weisse temporarily deployed to e2e November 2, 2023 11:08 — with GitHub Actions Inactive
@daniel-weisse daniel-weisse mentioned this pull request Nov 2, 2023
3 tasks
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.

Lgtm. I am not very familiar with the TF code right now, so I am requesting @elchead as well.

@derpsteb derpsteb requested a review from elchead November 7, 2023 08:36
@derpsteb
Copy link
Member

derpsteb commented Nov 7, 2023

Maybe it makes sense at some point to reuse the pattern Adrian introduced in the attestationconfig cli.

It allows unittesting the correct order of operations. And might simplify the apply function. I have sth like this in mind:

type Phase interface {
    func Apply() error
}

func NewPhases(skips map[skipPhase]{}) ([]Phase, error){
	// determine which phases to execute here.
	// call constructor for each phase.
        // don't use reflection, hardcode each constructor call.
}

func main() {
	phases, err := NewPhases(map[skipPhase]{})
	if err != nil {
		log.Fatal(err)
	}
	for _, phase := range phases {
		if err := phase.Apply(); err != nil {
			log.Fatal(err)
		}
	}
}

But I am not sure yet if this is needed. Most phases are just a single command. Imo the main goal would be to separate phase execution from determining which phase to run.

cli/internal/cmd/apply.go Outdated Show resolved Hide resolved
cli/internal/cmd/apply.go Outdated Show resolved Hide resolved
cli/internal/cmd/apply.go Outdated Show resolved Hide resolved
cli/internal/cmd/apply.go Outdated Show resolved Hide resolved
@@ -517,10 +530,18 @@ func (a *applyCmd) validateInputs(cmd *cobra.Command, configFetcher attestationc
cmd.PrintErrln("WARNING: Attestation temporarily relies on AWS nitroTPM. See https://docs.edgeless.systems/constellation/workflows/config#choosing-a-vm-type for more information.")
}

if !a.flags.skipPhases.contains(skipInitPhase) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is it just me, but I find this easier to read;
skip suggests we should skip it. (feel free to ignore)

Suggested change
if !a.flags.skipPhases.contains(skipInitPhase) {
if !a.flags.skipPhases.skip(skipInitPhase) {

@daniel-weisse
Copy link
Member Author

Updated the docs to remove create.
See the last two commits.

docs/docs/getting-started/first-steps-local.md Outdated Show resolved Hide resolved
docs/docs/getting-started/first-steps-local.md Outdated Show resolved Hide resolved
docs/docs/getting-started/first-steps-local.md Outdated Show resolved Hide resolved
docs/docs/getting-started/first-steps.md Outdated Show resolved Hide resolved
docs/docs/workflows/create.md Show resolved Hide resolved
daniel-weisse and others added 22 commits November 20, 2023 10:21
Signed-off-by: Daniel Weiße <[email protected]>
Signed-off-by: Daniel Weiße <[email protected]>
Signed-off-by: Daniel Weiße <[email protected]>
Signed-off-by: Daniel Weiße <[email protected]>
Signed-off-by: Daniel Weiße <[email protected]>
Signed-off-by: Daniel Weiße <[email protected]>
Signed-off-by: Daniel Weiße <[email protected]>
Signed-off-by: Daniel Weiße <[email protected]>
Signed-off-by: Daniel Weiße <[email protected]>
Signed-off-by: Daniel Weiße <[email protected]>
Signed-off-by: Daniel Weiße <[email protected]>
Signed-off-by: Daniel Weiße <[email protected]>
Signed-off-by: Daniel Weiße <[email protected]>
Signed-off-by: Daniel Weiße <[email protected]>
Copy link
Contributor

Coverage report

Package Old New Trend
cli/internal/cloudcmd 69.00% 68.40% ↘️
cli/internal/cmd 53.00% 54.00% ↗️
cli/internal/state 91.90% 94.60% ↗️

@daniel-weisse daniel-weisse merged commit 4c8ce55 into main Nov 20, 2023
10 checks passed
@daniel-weisse daniel-weisse deleted the feat/cli/create-apply branch November 20, 2023 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This introduces new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants