Conversation
Signed-off-by: OliverTrautvetter <66372584+OliverTrautvetter@users.noreply.github.com>
…phere-cloud/oms into cleanup_bootstrapped_infra
There was a problem hiding this comment.
Pull request overview
Adds a new bootstrap-gcp cleanup CLI subcommand to delete OMS-bootstrapped GCP projects (optionally cleaning up OMS-created DNS records), aiming to remove the need for manual teardown.
Changes:
- Add
oms beta bootstrap-gcp cleanupcommand with--project-id,--force, and--skip-dns-cleanupflags. - Label newly created GCP projects with
oms-managed=trueand add client methods to verify/delete OMS-managed projects and delete OMS DNS records. - Add docs and initial test scaffolding for the cleanup functionality.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
internal/bootstrap/gcp/gcp_client.go |
Adds OMS-managed labeling on project creation and implements delete/verification/DNS cleanup client methods. |
internal/bootstrap/gcp/mocks.go |
Updates generated mock to include new GCP client manager methods. |
internal/bootstrap/gcp/gcp_client_cleanup_test.go |
Adds new tests around constants/formatting (currently not exercising new production code paths). |
cli/cmd/bootstrap_gcp_cleanup.go |
Implements the new cleanup subcommand behavior (infra file loading, verification/prompt, DNS cleanup, project deletion). |
cli/cmd/bootstrap_gcp_cleanup_test.go |
Adds tests for opts defaults and flag wiring for the new command. |
cli/cmd/bootstrap_gcp.go |
Registers the new cleanup subcommand under bootstrap-gcp. |
docs/oms-cli_beta_bootstrap-gcp_cleanup.md |
Documents the new cleanup command and flags. |
docs/oms-cli_beta_bootstrap-gcp.md |
Adds the cleanup subcommand to the bootstrap-gcp docs index. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: OliverTrautvetter <66372584+OliverTrautvetter@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… and confirmation logic
Signed-off-by: OliverTrautvetter <66372584+OliverTrautvetter@users.noreply.github.com>
Signed-off-by: OliverTrautvetter <66372584+OliverTrautvetter@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: OliverTrautvetter <66372584+OliverTrautvetter@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: OliverTrautvetter <66372584+OliverTrautvetter@users.noreply.github.com>
Signed-off-by: OliverTrautvetter <66372584+OliverTrautvetter@users.noreply.github.com>
Signed-off-by: OliverTrautvetter <66372584+OliverTrautvetter@users.noreply.github.com>
Signed-off-by: OliverTrautvetter <66372584+OliverTrautvetter@users.noreply.github.com>
| if err := e.deps.FileIO.Remove(e.deps.InfraFilePath); err != nil { | ||
| log.Printf("Warning: failed to remove local infra file: %v", err) | ||
| } else { | ||
| log.Printf("Removed local infra file: %s", e.deps.InfraFilePath) | ||
| } |
There was a problem hiding this comment.
| if err := e.deps.FileIO.Remove(e.deps.InfraFilePath); err != nil { | |
| log.Printf("Warning: failed to remove local infra file: %v", err) | |
| } else { | |
| log.Printf("Removed local infra file: %s", e.deps.InfraFilePath) | |
| } | |
| if err := e.deps.FileIO.Remove(e.deps.InfraFilePath); err != nil { | |
| log.Printf("Warning: failed to remove local infra file: %v", err) | |
| return | |
| } | |
| log.Printf("Removed local infra file: %s", e.deps.InfraFilePath) |
| } | ||
|
|
||
| // cleanupExecutor manages state and logic for each cleanup step. | ||
| type cleanupExecutor struct { |
There was a problem hiding this comment.
I'm wondering if this type should be part of the internal/bootstrap/gcp package and export it's functions. That way you could as well test each function in isolation.
| log.Printf("Skipping DNS cleanup: missing base domain or DNS zone name (provide --base-domain/--dns-zone-name or use --skip-dns-cleanup)") | ||
| return nil | ||
| } | ||
| return e.deps.StepLogger.Step("Clean up DNS records", func() error { |
There was a problem hiding this comment.
this still looks like a bit of boilerplate just to use the step logger. I think the steplogger pattern being used in the bootstrapping command would be to call it in the ExecuteCleanup function, just passing a reference to this function to the steplogger.
There was a problem hiding this comment.
made an example comment in the ExecuteCleanup function for this specific function.
|
|
||
| _, err = client.SetIamPolicy(c.ctx, &iampb.SetIamPolicyRequest{ | ||
| Resource: resource, | ||
| Policy: policy, |
There was a problem hiding this comment.
will the policy still be valid when Bindings[x].Members is empty for any of the policies or will it return an error? I think in our case it's not important, since it won't be the case, just something I'm wondering :)
| return err | ||
| } | ||
|
|
||
| if err := exec.cleanupDNSRecords(); err != nil { |
There was a problem hiding this comment.
| if err := exec.cleanupDNSRecords(); err != nil { | |
| if err := exec.deps.StepLogger.Step("Clean up DNS records", exec.cleanupDNSRecords); err != nil { |
Add the ability to clean up the infrastructure I created with oms beta bootstrap-gcp , so that no manual cleanup is required.
Clickup