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 description field to VMs, Devices, Clusters Resource/Data #401

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

tagur87
Copy link
Contributor

@tagur87 tagur87 commented May 16, 2023

We were missing the ability to add a descriptions to devices, VMs, and
clusters. There are also corresponding data sources that do not have the
description field.

These fields were not available prior to netbox 3.4.3.

This adds the description field, following the example of the comment
field. The code was also slightly modified to be simpler to understand.

@tagur87 tagur87 marked this pull request as draft May 16, 2023 13:21
@fbreckle
Copy link
Collaborator

Note that, for VM things, you might also have to update the primary_ip_address resource.
I think right now the primary ip will just update the whole VM with all its attributes. Possibly this behavior can be fixed by using the Partial version of the api in

_, err = api.Virtualization.VirtualizationVirtualMachinesUpdate(updateParams, nil)

@tagur87
Copy link
Contributor Author

tagur87 commented May 16, 2023

Note that, for VM things, you might also have to update the primary_ip_address resource. I think right now the primary ip will just update the whole VM with all its attributes. Possibly this behavior can be fixed by using the Partial version of the api in

_, err = api.Virtualization.VirtualizationVirtualMachinesUpdate(updateParams, nil)

I had actually started to play with that. If we want to use partial, we may have to use omitempty on the parameter structs more, so that only the fields we want are sent.

@tagur87 tagur87 marked this pull request as ready for review July 19, 2023 14:00
@tagur87
Copy link
Contributor Author

tagur87 commented Jul 19, 2023

@fbreckle - i have updated this per your recommendation. Can you please review now?

@fbreckle
Copy link
Collaborator

Tests are failing

@tagur87
Copy link
Contributor Author

tagur87 commented Jul 19, 2023

Tests are failing

Yes but only on old versions. It works fine on newest version of netbox. Not sure what the difference could be?

@fbreckle
Copy link
Collaborator

Maybe similar to this? #427
I.e. the field was just not there in older versions? :D

@tagur87
Copy link
Contributor Author

tagur87 commented Jul 19, 2023

Maybe similar to this? #427 I.e. the field was just not there in older versions? :D

Gotcha....

according to netbox-community/netbox#11488 there were lots of description fields missing in the API. It was fixed in 3.4.3 it seems.

I agree the versioning problem is hard...seems like we should be able to release a new version with compatibility for version 3.4.3 and above, or something like that, and remove the tests for older versions. If people are running older netbox versions, they need to use the older provider.

@fbreckle
Copy link
Collaborator

Yes, that's the general idea. But that also means that people using older provider versions miss some features and bugfixes, so I do not want to cut them off just willy-nilly.
Since several resources are affected now and we can go ahead as you suggested. I plan to implement 3.5 support with 4.0, so that will be another cutoff anyway.

@tagur87
Copy link
Contributor Author

tagur87 commented Jul 20, 2023

Yes, that's the general idea. But that also means that people using older provider versions miss some features and bugfixes, so I do not want to cut them off just willy-nilly.
Since several resources are affected now and we can go ahead as you suggested. I plan to implement 3.5 support with 4.0, so that will be another cutoff anyway.

Ok, sounds good. I can submit a PR to remove the old versions and will see if there are other description fields missing that I can update at the same time as well.

tagur87 added a commit to tagur87/terraform-provider-netbox that referenced this pull request Jul 20, 2023
As discussed here:
e-breuninger#401 (comment)
we want to remove tests for versions before 3.4.3. This will enable us
to add description fields to many resources that were added in version
3.4.3 of netbox.
@tagur87
Copy link
Contributor Author

tagur87 commented Jul 20, 2023

depends on #433

fbreckle pushed a commit to tagur87/terraform-provider-netbox that referenced this pull request Jul 24, 2023
As discussed here:
e-breuninger#401 (comment)
we want to remove tests for versions before 3.4.3. This will enable us
to add description fields to many resources that were added in version
3.4.3 of netbox.
fbreckle pushed a commit that referenced this pull request Jul 24, 2023
As discussed here:
#401 (comment)
we want to remove tests for versions before 3.4.3. This will enable us
to add description fields to many resources that were added in version
3.4.3 of netbox.
@fbreckle
Copy link
Collaborator

With #433 merged, bump.

@tagur87
Copy link
Contributor Author

tagur87 commented Jul 24, 2023

@fbreckle - all tests have passed.

@tagur87 tagur87 force-pushed the vm-description branch 2 times, most recently from 3c3f496 to 0d19e56 Compare July 24, 2023 20:26
@tagur87
Copy link
Contributor Author

tagur87 commented Jul 24, 2023

Sorry about all the tab issues! Vscode being dumb.

@tagur87 tagur87 changed the title Add description field to resource_netbox_virtual_machine Add description field to VMs, Devices, Clusters Resource/Data Jul 26, 2023
We were missing the ability to add a descriptions to devices, VMs, and
clusters. There are also corresponding data sources that do not have the
description field.

These fields were not available prior to netbox 3.4.3.

This adds the description field, following the example of the comment
field. The code was also slightly modified to be simpler to understand.
@tagur87
Copy link
Contributor Author

tagur87 commented Jul 27, 2023

@fbreckle - i added few things to this. It should be ready now.

@fbreckle fbreckle merged commit 81d3099 into e-breuninger:master Jul 31, 2023
@tagur87 tagur87 deleted the vm-description branch July 31, 2023 21:01
twink0r pushed a commit to twink0r/terraform-provider-netbox that referenced this pull request Sep 15, 2023
As discussed here:
e-breuninger#401 (comment)
we want to remove tests for versions before 3.4.3. This will enable us
to add description fields to many resources that were added in version
3.4.3 of netbox.
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