Would like to have a "ignore_not_found" attribute on repository datasource

fscellos commented 8 months ago

Is there an existing issue for this?

[X] I have searched the existing issues

Community Note

Please vote on this issue by adding a :thumbsup: reaction to the original issue to help the community and maintainers prioritize this request
Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

In my use case i would like to apply a tf script that create repository if they don't exist in target nexus and import them if they already exist. Many types of repositories are managed through the tf script.

Repositories have to be created if some input var is set to true. In first intention, what i do is the following

import {
  for_each = var.npm_active ? ["1"] : []
  id       = format("%s-npm", upper(var.trigramme))
  to       = nexus_repository_npm_hosted.npm[0]
}
resource "nexus_repository_npm_hosted" "npm" {
  count  = var.npm_active ? 1 : 0
  name   = format("%s-npm", upper(var.trigramme))
  online = true
  storage {
    blob_store_name                = upper(var.trigramme)
    strict_content_type_validation = true
    write_policy                   = "ALLOW"
  }
}

but it doesn't work as expected if the repository doesn't exist : "could not read repository".

Another solution was to use datasource to calculate a boolean if repository doesn't exist and only apply import stage if it exist (else, simply apply ressource).

But it doesn't work because the Get function always return error on get (error could be 404 or 500, error is always send back)

repo, err := client.Repository.Npm.Hosted.Get(resourceData.Id()) if err != nil { return err }

So i would like to add an optional property "ignore_not_found" in datasource, used by above code, that have effect to map 404 error to nil one in return and keep actual behaviour for other.

I'm ok to implement it if you're agree to take this issue into account.

New or Affected Resource(s)/Data Source(s)

At least maven, npm, nuget, pypi, raw but all can be modified

Pro feature

[ ] Yes. Pro license Required

Community Plugin

No

Potential Terraform Configuration

data "nexus_repository_npm_hosted" "npml"{
  name = "RepoName"
  ignore_not_found = true
}

References

No response

fscellos commented 7 months ago

Hello. Any Mainteners out there to discuss about this issue ? (i wouldn't like to begin dev on it if i'm not sure to merge it at the end)

XREvo commented 7 months ago

I tried to implement it today. Here is the branch on my fork: https://github.com/XREvo/terraform-provider-nexus/tree/allow-404-response

I added the flag ignore_not_found on each datasource and set up a simple mechanism to detect if API response error must/can be managed:

ignore_not_found, ok := resourceData.Get("ignore_not_found").(bool)
if !ok {
  ignore_not_found = false
}

repo, err := client.Repository.Conan.Proxy.Get(resourceData.Id())
if err != nil {
  if !(ignore_not_found && strings.Contains(err.Error(), "HTTP: 404")) {
    return err
  }
}

But the unit test that i added keep failing with:

--- FAIL: TestAccDataSourceRepositoryAptHostedNotFound (0.98s)
    testing_new_config.go:58: no "id" found in attributes
    testing_new.go:63: no "id" found in attributes
FAIL

This behavior is known on the Terraform's documentation to be met when you do not write a value in the "id" attribute of your state. And as far as I understand an empty string is not considered as a value, because if I inject a random string in the "id" attribute, the test pass.

FYI, the code used to read the data source use this code if we "bypass" the 404 error as previously stated:

if repo == nil {
  resourceData.SetId("")
  return nil
}

Unfortunately @fscellos I don't think we can implement it like you asked.

Maybe another approach would be to use a dedicated data source to find if a repository exists. I'm thinking of a data source like this one :

data "nexus_repository_apt_hosted_exists" "my-repo-exists" {
    name   = 'my-repo-name"
}

It would use the value of "name" as value for "id" and output a "exists" boolean flag to check if it exists or not.

What do you think of it?

anmoel commented 7 months ago

hello @fscellos , hi @XREvo,

thank you for your input. i think it would be better to implement a new data source with name "nexus_repositories" that returns a list of all repositories. Your problem can be fixed like this:

locals {
  npm_repos = [
    "test1",
    "test2",
  ]
  import_npm_repos = { for repo in data.nexus_repositories.all.list : repo.name  => repo if !contains(local.npm_repos, repo.name) }
}

data "nexus_repositories" "all" {
}

import {
  for_each = local.import_npm_repos
  id       = each.value.name
  to       = nexus_repository_npm_hosted.npm[each.key]
}
resource "nexus_repository_npm_hosted" "npm" {
  for_each = local.npm_repos
  name   = each.key
  online = true
  storage {
    blob_store_name                = upper(var.trigramme)
    strict_content_type_validation = true
    write_policy                   = "ALLOW"
  }
}

with this solution it can be implemented with all types of repositories and in the future we can add search arguments to this data resource

fscellos commented 7 months ago

Hello @anmoel , @XREvo

As the nexus go-client used doesn't support yet search API i think using what you suggest (filtering datasources) is a good tradeoff.

Maybe, we can just add an optional "type" filter on this data source in order to control amount of data manipulated in terraform script (even if your example is ok for our use case) (this type data is send back in RepositoryInfo structure from nexus client).

As we already plan this feature in our internal board, i think we'll be able to submit you a PR in a couple of days.

fscellos commented 7 months ago

Hello,

It appears that the suggested DataSource already exist in the provider (ie. "nexus_repository_list"). I just see it as i read the code.

But in my use case i also need to have such a feature for Blob stores, for which no datasource of type "list" exist.

It's not directly link to this issue but i push a PR to introduce, instead, a "nexus_blobstore_list". : PR 442 Fabrice.

anmoel commented 5 months ago

Hi @fscellos,

your PR was released in version 2.3.0

datadrivers / terraform-provider-nexus