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

Proposal: Switch from fbreckle/go-netbox to netbox-community/go-netbox #591

Open
yoo opened this issue May 12, 2024 · 3 comments
Open

Proposal: Switch from fbreckle/go-netbox to netbox-community/go-netbox #591

yoo opened this issue May 12, 2024 · 3 comments

Comments

@yoo
Copy link

yoo commented May 12, 2024

Currently the provider uses fbreckle/go-netbox as the NetBox API client.
I'd like to propose to switch to netbox-community/go-netbox.

The readme of fbreckle/go-netbox states, it's maintained because of this terraform provider.
If a resource or field is missing, it needs to be added to the client first.
I encountered this while working on #538.

The switch would allow the provider to benefit from the work done by the netbox-community.

Gradual Switchover

One concern stated in fbreckle/go-netbox is that every API call needs to be
changed to use the new client. This is true, but can be done gradually. The two clients
can be used in parallel and the resources can be switched one by one.

First introduce the new client and deprecate the old one.
The following struct is passed to the provider functions as meta argument.

type Config struct {}
	LegacyClient *legacynetbox.NetBoxAPI
	Client *netbox.ApiClient
}

This requires a change to every existing resource, but can be done by a simple search and replace.

api := m.(*client.NetBoxAPI)
// becomes
api := m.(*Config).LegacyClient

After the introduction of the new client, the resources can be switched one by one.
The new type assertion would read:

api := m.(*Config).Client

Request Cancellation

In addition the new API client can work with contexts, allowing for request cancellation.

func dataSourceNetboxRacksRead(ctx context.Context, d *schema.ResourceData, m interface{}) error {
	api := m.(*Config).Client

	req := api.DcimAPI.DcimRacksList(ctx)

	if limitValue, ok := d.GetOk("limit"); ok {
		req = req.Limit(int32(limitValue.(int)))
	}
    [...]

If this is something you are interested in I'd like to open a PR introducing the new API client.
Naming and other code details can than be discussed in the PR.
Maybe give a 👍 or 👎 to see how people feel about this.

@mraerino
Copy link
Contributor

i really like this proposal as it will allow to modernize the provider in a gradual way.

@fbreckle what are your thoughts on this?

@fbreckle
Copy link
Collaborator

fbreckle commented Sep 2, 2024

Generally, I am absolutely in favor of migrating to a not self-managed version of go-netbox and I also absolutely appreciate the work being done in this MR, ...

BUT! Lets look at a timeline here.

Netbox 4.0 released on May 7th.
As you can see, this MR is from May 12th.
netbox-community/go-netbox was updated to 4.0.x on May 31th.

So, when I initially got this MR, the community go-netbox was not 4.0 compatible. This was a problem because I cannot just make changes on the community go-netbox as I can do in my fork and 4.0 compatibility was one of the changes I wanted to focus on rather quickly (we all know that this took way longer then, but I did not plan for this).

So initially, I did not see the point in merging this MR if the upstream library does not fulfill the needs of the provider.
Fast forward to today. The latest community go-netbox is still from May 31th. I do not want to migrate to that library only to find myself forking it once again.

tl;dr: In theory, I very much like this MR. In practice, I wonder if migrating to community go-netbox in its current state is a smart move.

@mraerino
Copy link
Contributor

mraerino commented Sep 3, 2024

i found myself using oapi-codegen to generate a client directly from the OpenAPI 3 spec that netbox provides. the generated code is very modular and might serve us better. what do you think about moving to such a setup?

It might be good to move away from the static swagger 2.0 spec that is heavily patched, since netbox also uses openapi 3 now. this could make it easier to keep track of netbox changes.

here is an example setup: https://github.com/ffddorf/terraform-provider-netbox-bgp/tree/main/client

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

No branches or pull requests

3 participants