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.

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

cli: remove/refactor upgrade package #2266

Merged
merged 8 commits into from
Aug 23, 2023
Merged

cli: remove/refactor upgrade package #2266

merged 8 commits into from
Aug 23, 2023

Conversation

daniel-weisse
Copy link
Member

@daniel-weisse daniel-weisse commented Aug 21, 2023 •
edited
Loading

Context

The upgrade package does not need to exist, since the code fits well into cloudcmd.
Additionally, code in the package contains a lot of duplication and could use a general clean up.

Proposed change(s)

  • Remove the upgrade package in cli/internal/upgrade
    • Existing code move to cloudcmd package in cli/internal/cloudcmd
  • Unify and simplify logic for Terraform IAM and cluster upgrades
  • Write Terraform IAM variables using hcl package, instead of using custom string methods
  • Add some more unit tests

@daniel-weisse daniel-weisse added the no changelog Change won't be listed in release changelog label Aug 21, 2023
@netlify
Copy link

netlify bot commented Aug 21, 2023 •
edited
Loading

✅ Deploy Preview for constellation-docs canceled.

Name Link
🔨 Latest commit d1ee331
🔍 Latest deploy log https://app.netlify.com/sites/constellation-docs/deploys/64e5a5da9bdd3500084c5a4d

@elchead
Copy link
Contributor

elchead commented Aug 21, 2023

did you run an E2E upgrade?
I see the pipeline failed recently:
https://github.com/edgelesssys/constellation/actions/runs/5924543669/job/16062252001#step:8:415

@daniel-weisse
Copy link
Member Author

I see the pipeline failed recently:

This is unrelated to this PR, and the problem was already fixed here: #2262

I'll rerun the test once a couple other PRs are merged, and I have rebased this branch

elchead
elchead reviewed Aug 21, 2023
View reviewed changes
cli/internal/cmd/iamupgradeapply.go
@@ -6,11 +6,14 @@ SPDX-License-Identifier: AGPL-3.0-only
package cmd

import (
"context"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you test that IAM upgrades still works (e.g. remove one permission and check if diff is applied)? Writing a test still seems different in the current code stage but would ofc be more sustainable

Copy link
Member Author

Choose a reason for hiding this comment

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

Manual tests ran without problem.
I will start a couple automated upgrade tests later.

@elchead
Copy link
Contributor

elchead commented Aug 21, 2023

like the design changes, good job!

@daniel-weisse daniel-weisse force-pushed the ref/cli/upgrade-pkg branch 2 times, most recently from efd33b3 to a8e06b8 Compare August 22, 2023 09:13
@daniel-weisse
Move IAM migration client to cloudcmd package
16a13a8 
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
@daniel-weisse
Move Terraform Cluster upgrade client to cloudcmd package
f275a62 
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
@daniel-weisse
Use hcl for creating IAM var files
11a3c54 
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
@daniel-weisse
Unify terraform upgrade code
2db3e7d 
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
@daniel-weisse
Clean up comments
479fc2a 
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
@daniel-weisse
Update doc comments
c628d1f 
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
@daniel-weisse
Rename cloudcmd files for better clarity
3823727 
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
@daniel-weisse
Regenerate Bazel build files
d1ee331 
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
@daniel-weisse
Copy link
Member Author

daniel-weisse commented Aug 23, 2023 •
edited
Loading

e2e upgrade test Azure ✔️
e2e upgrade test GCP ✔️
e2e upgrade test AWS ✔️

@elchead
Copy link
Contributor

elchead commented Aug 23, 2023

sorry just realized that my review was pending

elchead
elchead approved these changes Aug 23, 2023
View reviewed changes
Copy link
Contributor

@elchead elchead left a comment

Choose a reason for hiding this comment

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

when the upgrade tests are green, lgtm

derpsteb
derpsteb approved these changes Aug 23, 2023
View reviewed changes
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.

Lovely.
Didn't do a detailed pass as it seems like Adrian already did.

@daniel-weisse daniel-weisse merged commit 0a91180 into main Aug 23, 2023
14 checks passed
@daniel-weisse daniel-weisse deleted the ref/cli/upgrade-pkg branch August 23, 2023 08:35
msanft pushed a commit that referenced this pull request Sep 1, 2023
@daniel-weisse @msanft
cli: remove/refactor upgrade package (#2266)
e5ef8bb 
* Move IAM migration client to cloudcmd package

* Move Terraform Cluster upgrade client to cloudcmd package

* Use hcl for creating Terraform IAM variables files

* Unify terraform upgrade code

* Rename some cloudcmd files for better clarity

---------

Signed-off-by: Daniel Weiße <dw@edgeless.systems>
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

Assignees
No one assigned
Labels
no changelog Change won't be listed in release changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@daniel-weisse @elchead @derpsteb