-
Notifications
You must be signed in to change notification settings - Fork 192
refactor: Supports cleaning resources for static projects with test-acc-tf-p-keep
prefix
#3269
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
Conversation
… but have resources removed
@@ -37,6 +37,9 @@ var ( | |||
keptPrefixes = []string{ | |||
"test-acc-tf-p-keep", | |||
} | |||
keepProjectEmptyPrefixes = []string{ | |||
"test-acc-tf-p-empty", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any case where we want to keep the project without deleting anything in it? are they with prefix test-acc-tf-p-keep? also I can imagine that in keep ones we want to keep some type of resource (e.g. private endpoint) but delete others (like clusters). so not sure if it will be more general to use only one prefix project, e.g. test-acc-tf-p-keep, and then use "keep" in resources we want to keep inside, e.g. cluster "test-acc-tf-c-keep-mycluster would me not to delete that cluster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree we can preserve test-acc-tf-p-keep
prefix for all cases.
Not sure how complex it would be to start adding different keep
prefixes for each resource type, as for this PR maybe we can stick to test-acc-tf-p-keep
(preserves any resources) and test-acc-tf-p-keep-empty
(removes all resources)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AgustinBettati FYI, see also comments in the doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed test-acc-tf-p-keep
instead
@@ -37,6 +37,9 @@ var ( | |||
keptPrefixes = []string{ | |||
"test-acc-tf-p-keep", | |||
} | |||
keepProjectEmptyPrefixes = []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes LGTM, would only double check that for the following projects we don't have any static resources we are relying on that could be deleted. (I dont believe its the case)
EAR-example (6792ac6ceaa5601d0f45dc95)
test-acc-tf-p-keep-ear-AWS-private-endpoint (6790e57a9b41416f5c216fee)
test-acc-tf-p-keep-ear-private-endpoint (66d83bcc1fe1835125c52422)
test-acc-tf-p-keep-encryption-at-rest (67810c93a7dcf40b5f0db4c2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked. Only test-acc-tf-p-keep-ear-AWS-private-endpoint
has a few steam_instances, but those are ok to delete
This PR has gone 7 days without any activity and meets the project’s definition of "stale". This will be auto-closed if there is no new activity over the next 7 days. If the issue is still relevant and active, you can simply comment with a "bump" to keep it open, or add the label "not_stale". Thanks for keeping our repository healthy! |
…nly found a few stream-instances for now)
test-acc-tf-p-keep
prefix
@@ -204,11 +211,6 @@ func removeProjectResources(ctx context.Context, t *testing.T, dryRun bool, clie | |||
} | |||
|
|||
func projectSkipReason(p *admin.Group, skipProjectsAfter time.Time, onlyEmpty bool) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the difference between projectSkipReason and projectNoDeleteReason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
projectSkipReason avoids cleaning the project alltogether. (All non bot projects are not cleaned by default)
skipProjectDelete cleans resources but doesn't delete the project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the clarification
Co-authored-by: Leo Antoli <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a suggestion for improving clarity
t.Logf("will try to delete %d projects:", len(projectsToDelete)) | ||
slices.Sort(projectInfos) | ||
t.Log(strings.Join(projectInfos, "\n")) | ||
var deleteErrors int | ||
var emptyProjectCount int | ||
for name, projectID := range projectsToDelete { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we adjust the log to mention the current behaviour? "deleting project resources and optionally delete project"
same for variable projectsToDelete
maybe projectsToCleanAndDelete
makes it more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Changed in 975741b
…roject resource management
Description
Related issue: https://jira.mongodb.org/browse/CLOUDP-314965
Supports cleaning projects with
"test-acc-tf-p-keep"
prefix, that shouldn't be deleted but have resources removed when running the cleanup (the project should be empty after tests).We have various tests that depend on using specific projects with feature flags enabled.
However, these projects risk having extra resources that are not cleaned.
The only resources found for deletion:
Sweeper Alternative considered
tl;dr: Uses the same mechanism as
go test
but with a-sweep={regionName}
flag. I consider our current cleanup test to be easier to adapt than adding custom sweepers and a new CI workflow.CheckDestroy
: If a test panics or the process terminates unexpectedly, checkDestroy never runs.Type of change:
Required Checklist:
Further comments