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

Have a force_delete true/false on repositories similar to AWS ECR. #1069

Open
EricChen1248 opened this issue Sep 9, 2024 · 5 comments
Open
Assignees
Labels
enhancement New feature or request

Comments

@EricChen1248
Copy link

Is your feature request related to a problem? Please describe.
Deleting a non empty repository is too easy and destructive.

If a misconfiguration or something is made that causes terraform to decide to delete a repo, and if there contents in the repositories, the contents are also deleted non reversibly.

Describe the solution you'd like
Have a setting similar to AWS's ECR to allow/disable "force delete"
I expect terraform apply/destroy to fail to delete a repo that has content in it unless explictly set to force delete.

Describe alternatives you've considered
No alternatives. Terraform is too dangerous for us to use with this behavior.

Additional context
Add any other context or screenshots about the feature request here.

@EricChen1248 EricChen1248 added the enhancement New feature or request label Sep 9, 2024
@alexhung
Copy link
Member

alexhung commented Sep 9, 2024

@EricChen1248 Thanks for the suggestion. I think this is very good idea and I'll add this to our plan.

@alexhung
Copy link
Member

alexhung commented Sep 20, 2024

@EricChen1248 I have a few questions:

  1. Is this deletion behavior only for local repository, since it is the only type that Artifactory stores artifacts?
  2. The force_delete attribute in AWS ECR resource is supported by AWS REST API, i.e. there's a corresponding force field in the DeleteRepository. There is no such support in Artifactory currently, so the implementation/behavior will be different.
  3. Terraform supports "dry run" by using terraform plan -destroy (https://developer.hashicorp.com/terraform/cli/commands/destroy#usage) and let you preview resources to be destroyed. Is this not sufficient for your use case?

While I think it is a good idea to perform additional checks before deleting a repository, and makes it explicit for users to delete existing repository, it is not as straightforward as just adding a new attribute and a conditional check.

For example, to ensure backward compatibility, this attribute will need to be defaulted to true initially. This means you will have to set force_delete = false explicitly for every repository resources in your configuration. Is this a reasonable ask for the initial implementation? It's 50:50 for me at this point.

@jgrumboe
Copy link

Hi @alexhung
I just saw that v12.1.0 has an allow_delete flag now. Cool feature, but terraform validate fails with latest provider version with Error: Unsupported argument.

@alexhung
Copy link
Member

@jgrumboe This is still in progress. The doc update must have slipped through into one of the recent PR. I'll fix the doc in next release.

@EricChen1248
Copy link
Author

  1. Personally, yes, only local repositories. Technically remotes can be impacted with it's cache, but that's probably less a priority, makes less sense to check if non empty, and if it's a remote that people care enough to perpetually fix contents with, they should probably created a local mirror/snapshot for it anyways.
  2. I hoped it would be something that also has a corresponding change on the REST side, but barring that, makes sense.
  3. No, dry run only tells me it is going to destroy the repo, but doesn't tell me there are content in the repo that is also going to be irrecoverably deleted. (Send deleted repository resources to Artifactory trash when destroying #1094 mitigates this somewhat, but still less optimal, as recovery is still difficult)

For an initial implementation, I see that making sense. Backwards compatibility is important. Perhaps to make it more useful, allowing it to be a provider level setting that can be overwritten per repo would make easing into it easier for people who want to use it.

provider "artifactory" {
  url          = var.artifactory_url
  access_token = var.artifactory_access_token
  delete_non_empty_repo = false
}


resource "artifactory_local_generic_repository" "repo-that-allows-delete" {
  key = "repo1"
  force_delete = true
}

resource "artifactory_local_generic_repository" "repo-that-disallow-delete" {
  key = "repo2"
}

In this case, repo1 can be deleted even if it's non empty, repo2 has no settings so it defaults to what's set in the provider. And then provider.artifactory.delete_non_empty_repo defaults to true if nothing is set.

(Not sure if this is something doable or makes sense for Terraform, only coming at it from a user POV)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants