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

feat add force destroy to environment resource #175

Merged
merged 48 commits into from
Dec 9, 2021

Conversation

alonnoga
Copy link
Contributor

@alonnoga alonnoga commented Dec 8, 2021

Issue & Steps to Reproduce / Feature Request

add force destroy safeguard to environment resource

@alonnoga alonnoga self-assigned this Dec 8, 2021
@alonnoga alonnoga requested a review from a team December 8, 2021 14:31
@alonnoga
Copy link
Contributor Author

alonnoga commented Dec 8, 2021

"name": environment.Name,
"project_id": environment.ProjectId,
"template_id": environment.LatestDeploymentLog.BlueprintId,
"force_destroy": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

because now destroy requires "force_destroy" I had to add it to all the tests for test cleanup to work

Comment on lines +256 to +260
canDestroy := d.Get("force_destroy")

if canDestroy != true {
return diag.Errorf(`must enable "force_destroy" safeguard in order to destroy`)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

do you sure about it? this means if the user didn't set force_destroy: true he can't destroy this environment

Copy link
Contributor Author

@alonnoga alonnoga Dec 9, 2021

Choose a reason for hiding this comment

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

yea, thats what @avnerenv0 and @yaronya wanted
They said thats how it is in s3 tf provider
Personally I don't like it

Copy link
Contributor

Choose a reason for hiding this comment

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

mozar - The user should know when he does destroy the data will be deleted

Copy link
Contributor

Choose a reason for hiding this comment

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

@avnerenv0 do we sure about it for destory?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah man we had like 50 discussions about this already.

This concept comes from the aws_s3_bucket resource and is pretty standard and accepted in TF.
Also exists for google_storage_bucket, for example.

There is also prevent_destroy concept but we'd like to go with the 'safer' option.

Base automatically changed from feat-add-ttl-to-environment to main December 9, 2021 09:59
@alonnoga alonnoga merged commit 5d4637a into main Dec 9, 2021
@alonnoga alonnoga deleted the feat-add-force-destroy-to-environment-resource branch December 9, 2021 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants