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

Add support for physical and shipping address in sites #337

Merged
merged 1 commit into from
Feb 9, 2023

Conversation

Ikke
Copy link
Contributor

@Ikke Ikke commented Feb 8, 2023

No description provided.

@Ikke Ikke changed the title feat: Add support for physical and shipping address in sites Add support for physical and shipping address in sites Feb 8, 2023
@fbreckle
Copy link
Collaborator

fbreckle commented Feb 8, 2023

Looks good so far. Can you add a test that also unsets the fields and see if that works? You might have to set the string to ' ' to unset it.

@Ikke Ikke force-pushed the site-add-addresses branch from 66cf76d to 03782e4 Compare February 9, 2023 09:03
@Ikke
Copy link
Contributor Author

Ikke commented Feb 9, 2023

@fbreckle I adjusted it to use the same approach as description. I assume you are aware that setting it explicitly to an empty string in terraform would not work due to the omitempty on those fields in get-netbox library

@fbreckle
Copy link
Collaborator

fbreckle commented Feb 9, 2023

Yes, but a single space is not an empty string :D

@Ikke
Copy link
Contributor Author

Ikke commented Feb 9, 2023

Yes, but a single space is not an empty string :D

Yes, indeed 🙂. So this solution works when you leave out the field, but doing something like

resource "netbox_site" "test" {
  name             = "test"
  physical_address = ""
}

will not update physical address, and

resource "netbox_site" "test" {
  name             = "test"
  physical_address = " "
}

will keep showing up as a change (as netbox trims the field).

It's just something practitioners need to be aware of.

@fbreckle
Copy link
Collaborator

fbreckle commented Feb 9, 2023

I suppose that applies to the other string fields where it is implemented like that as well? No issue on my side.

@Ikke
Copy link
Contributor Author

Ikke commented Feb 9, 2023

I suppose that applies to the other string fields where it is implemented like that as well? No issue on my side.

Yes, that would apply there as well.

@fbreckle fbreckle merged commit 0c22a9c into e-breuninger:master Feb 9, 2023
@Ikke Ikke deleted the site-add-addresses branch February 9, 2023 19:58
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.

2 participants