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

Chore: add soft delete to configuration variable #866

Conversation

alonnoga
Copy link
Contributor

@alonnoga alonnoga commented May 30, 2024

Issue & Steps to Reproduce / Feature Request

Part of :#856

We need to be able to remove configuration variables from code without actually deleting them

In environment import we initialize variables for the user to review and create an environment from
These resources are not expected to be committed, managed other then applying them once, so we need them to not be removed once the configuration is removed

Edit:
After adding a soft delete flag to config variable it I realized we should have added one to env import resource to make this strange behavior explicit 👍🏻 ( So I added that as well )

@@ -110,6 +110,11 @@ func resourceConfigurationVariable() *schema.Resource {
Description: "the value of this variable must match provided regular expression (enforced only in env0 UI)",
Optional: true,
},
"soft_delete": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TomerHeber now that I think about it we could have added it to "environment import resource" as well so that it won't be implicit that its not actually deleted

@alonnoga alonnoga requested a review from TomerHeber May 30, 2024 16:50
"soft_delete": {
Type: schema.TypeBool,
Description: "soft delete the configuration variable, once removed from the configuration it won't be deleted from env0",
Optional: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a default value. This prevents many issues with bool types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what are the issues?

@@ -237,6 +242,11 @@ func resourceConfigurationVariableUpdate(ctx context.Context, d *schema.Resource
}

func resourceConfigurationVariableDelete(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
// don't delete if soft delete is set
if softDelete, ok := d.GetOk("soft_delete"); ok && softDelete.(bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you put a default you can just get the softDelete.
GetOk does not work properly with booleans...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"soft_delete": {
Type: schema.TypeBool,
Description: "soft delete the configuration variable, once removed from the configuration it won't be deleted from env0",
Optional: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here - please add a default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

func resourceEnvironmentImportDelete(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
// don't delete if soft delete is set
if softDelete, ok := d.GetOk("soft_delete"); ok && softDelete.(bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here.. once you have a default.. just get the value...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@TomerHeber TomerHeber left a comment

Choose a reason for hiding this comment

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

LGTM - just a few small changes required.

@alonnoga alonnoga requested a review from TomerHeber June 5, 2024 12:23
@@ -58,6 +58,12 @@ func resourceEnvironmentImport() *schema.Resource {
Description: "iac version of the environment",
Optional: true,
},
"soft_delete": {
Type: schema.TypeBool,
Description: "soft delete the configuration variable, once removed from the configuration it won't be deleted from env0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this comment correct? default is false...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah if they set the field ( to true ) it won't delete
I just defaulted it to false so it won't be set by default ( we want soft delete to be explicit )

we generate the code that uses these flags so we'll just set soft_delete = true we don't really expect customers to use those flags, but we just didn't want the resource to act in unexpected ways without it being explicit

@alonnoga
Copy link
Contributor Author

alonnoga commented Jun 5, 2024

@TomerHeber please also see this commit 22a1e2e

@github-actions github-actions bot added ready to merge PR approved - can be merged once the PR owner is ready and removed pending final review labels Jun 5, 2024
@alonnoga alonnoga merged commit d5d18eb into main Jun 5, 2024
4 checks passed
@alonnoga alonnoga deleted the chore-add-a-flag-for-not-actually-deleting-configuration-variables-on-removal-#856 branch June 5, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-client chore provider ready to merge PR approved - can be merged once the PR owner is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants