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

fix(setup): do not create field which already exist #2953

Merged
merged 10 commits into from
Mar 27, 2023

Conversation

sumo-drosiek
Copy link
Contributor

@sumo-drosiek sumo-drosiek commented Mar 24, 2023

If we get field which is non lowercase, we try to recreate it, which leads to errors and setup process fail

Fixes #2865 proper way

Checklist

  • Changelog updated or skip changelog label added
  • Documentation updated
  • Template tests added for new features
  • Integration tests added or modified for major features

Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
@sumo-drosiek sumo-drosiek requested a review from a team as a code owner March 24, 2023 08:21
@sumo-drosiek sumo-drosiek changed the title fix(setup): stop being case sensitive in fields creation process fix(setup): do not create field which already exist Mar 24, 2023
Dominik Rosiek added 4 commits March 24, 2023 09:31
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
@swiatekm
Copy link

What does this change actually do? It seems like it imports the field even if it's uppercase - but then won't we actually lowercase it when applying? That doesn't sound like the right thing to do here.

@sumo-drosiek
Copy link
Contributor Author

sumo-drosiek commented Mar 24, 2023

What does this change actually do? It seems like it imports the field even if it's uppercase - but then won't we actually lowercase it when applying? That doesn't sound like the right thing to do here.

You may be right. I will try to research the following solution:
change var.create_fields to be a map, so every field can be managed separately. WDYT?

I found that we can simply ignore name changes: https://developer.hashicorp.com/terraform/language/meta-arguments/lifecycle#ignore_changes

Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
@swiatekm
Copy link

What does this change actually do? It seems like it imports the field even if it's uppercase - but then won't we actually lowercase it when applying? That doesn't sound like the right thing to do here.

You may be right. I will try to research the following solution: change var.create_fields to be a map, so every field can be managed separately. WDYT?

I think that if we're going to require special configuration for this to work, the user may as well just make the field uppercase in their values.yaml. Unless I'm misunderstanding your suggestion.

I found that we can simply ignore name changes: https://developer.hashicorp.com/terraform/language/meta-arguments/lifecycle#ignore_changes

That would actually work, I think. I'm a bit hesitant about this change in general, because I don't think we have a way of easily testing it. I don't think you can create an uppercase field in Sumo right now.

Dominik Rosiek added 2 commits March 24, 2023 14:10
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
@sumo-drosiek
Copy link
Contributor Author

That would actually work, I think. I'm a bit hesitant about this change in general, because I don't think we have a way of easily testing it. I don't think you can create an uppercase field in Sumo right now.

I was thinking about it and I will manually import some field with different name and see what will happen during plan/apply

Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
@sumo-drosiek
Copy link
Contributor Author

Tested:

➜  terraform terraform apply # with ignore_changes
sumologic_field.cluster: Refreshing state... [id=zzz]
sumologic_field.cluster2: Refreshing state... [id=aaa]

No changes. Your infrastructure matches the configuration.

Terraform has compared your real infrastructure against your configuration and found no
differences, so no changes are needed.

Apply complete! Resources: 0 added, 0 changed, 0 destroyed.
➜  terraform terraform plan # without ignore changes
sumologic_field.cluster: Refreshing state... [id=zzz]
sumologic_field.cluster2: Refreshing state... [id=aaa]

Terraform used the selected providers to generate the following execution plan. Resource
actions are indicated with the following symbols:
-/+ destroy and then create replacement

Terraform will perform the following actions:

  # sumologic_field.cluster2 must be replaced
-/+ resource "sumologic_field" "cluster2" {
      ~ field_id   = "zzz" -> (known after apply)
      ~ field_name = "service" -> "clusters" # forces replacement
      ~ id         = "aaa" -> (known after apply)
        # (2 unchanged attributes hidden)
    }

Plan: 1 to add, 0 to change, 1 to destroy.

Comment on lines 5 to 7
lifecycle {
ignore_changes = [field_name, data_type]
}

Choose a reason for hiding this comment

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

A comment here explaining why this exists would be very helpful. Otherwise LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment with description and link to #2865

Signed-off-by: Dominik Rosiek <drosiek@sumologic.com>
@sumo-drosiek sumo-drosiek merged commit 59d917c into main Mar 27, 2023
@sumo-drosiek sumo-drosiek deleted the drosiek-fix-non-lower-case-fields branch March 27, 2023 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup job fails trying to create fields that already exist
2 participants