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: write infrastructure to new state file #2321

Merged
merged 7 commits into from
Sep 25, 2023

Conversation

elchead
Copy link
Contributor

@elchead elchead commented Sep 8, 2023

Context

This is the first implementation part of the new constellation state (#2281).

Proposed change(s)

create

  • aggregate infrastructure state from TF output
  • write the infrastructure output to constellation-state.yaml with version v1 after terraform apply

upgrade

  • extend TF client with ShowInfrastrucutre() to output the infrastructure part of the state
  • use ShowInfrastructure() and overwrite the infrastructure output to constellation-state.yaml with version v1 after terraform apply

Related issue

Checklist

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

@netlify
Copy link

netlify bot commented Sep 8, 2023

Deploy Preview for constellation-docs canceled.

Name Link
🔨 Latest commit e603c2e
🔍 Latest deploy log https://app.netlify.com/sites/constellation-docs/deploys/65117c254938cb0008506073

@elchead elchead changed the title use infra state in upgrade cli: write new infrastructure state Sep 8, 2023
@elchead elchead added the no changelog Change won't be listed in release changelog label Sep 8, 2023
@elchead elchead force-pushed the feat/cli/infrastructure-state branch from 3336fdf to 0dee6aa Compare September 8, 2023 08:12
@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2023

Coverage report

Package Old New Trend
cli/internal/cloudcmd 64.20% 63.90% ↘️
cli/internal/cmd 55.90% 56.10% ↗️
cli/internal/helm 49.50% 49.50% 🚧
cli/internal/state 0.00% [no test files] 🚨
cli/internal/terraform 71.40% 68.50% ↘️
internal/constants [no test files] [no test files] 🚧

@elchead elchead changed the title cli: write new infrastructure state cli: write infrastructure to new state file Sep 8, 2023
@elchead elchead force-pushed the feat/cli/infrastructure-state branch from 0dee6aa to 61747c3 Compare September 8, 2023 09:24
@elchead elchead marked this pull request as ready for review September 8, 2023 09:45
@3u13r 3u13r added this to the v2.12.0 milestone Sep 8, 2023
@elchead elchead force-pushed the feat/cli/infrastructure-state branch 2 times, most recently from 2355be8 to d4aa307 Compare September 11, 2023 11:22
@derpsteb
Copy link
Member

LGTM. Would just like to discuss that one point.

@derpsteb
Copy link
Member

Before merging this: what is the plan to ensure that the migration from the idfile to the statefile happens within 1 release cycle? How do we migrate?

@elchead
Copy link
Contributor Author

elchead commented Sep 18, 2023

Before merging this: what is the plan to ensure that the migration from the idfile to the statefile happens within 1 release cycle? How do we migrate?

The idFile is only removed in the last step (see user stories in Azure). So the code is not broken at any point.
When we prioritize this story, it should be very feasible to merge this in the next release.
Otherwise, we would have both the idFile and state file during the intermediate release.

@derpsteb
Copy link
Member

Good to merge from my side.

@elchead elchead force-pushed the feat/cli/infrastructure-state branch from cd3a85b to d206a59 Compare September 21, 2023 13:39
cli/internal/cmd/create.go Outdated Show resolved Hide resolved
cli/internal/cmd/upgradeapply.go Outdated Show resolved Hide resolved
cli/internal/cmd/upgradeapply.go Outdated Show resolved Hide resolved
cli/internal/state/state.go Show resolved Hide resolved
cli/internal/state/state.go Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Doc comments in this file need some work, but should probably be handled in AB#3426

cli/internal/terraform/terraform.go Show resolved Hide resolved
cli/internal/state/state.go Show resolved Hide resolved
3u13r
3u13r previously requested changes Sep 25, 2023
cli/internal/state/state.go Outdated Show resolved Hide resolved
cli/internal/cmd/upgradeapply.go Outdated Show resolved Hide resolved
@elchead elchead requested a review from 3u13r September 25, 2023 12:24
@elchead elchead force-pushed the feat/cli/infrastructure-state branch from 5ce0d30 to e603c2e Compare September 25, 2023 12:25
Copy link
Member

@3u13r 3u13r left a comment

Choose a reason for hiding this comment

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

LGTM

@elchead elchead merged commit 322c4aa into main Sep 25, 2023
7 checks passed
@elchead elchead deleted the feat/cli/infrastructure-state branch September 25, 2023 14:19
@3u13r 3u13r mentioned this pull request Sep 27, 2023
2 tasks
msanft pushed a commit that referenced this pull request Sep 27, 2023
derpsteb pushed a commit that referenced this pull request Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Change won't be listed in release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants