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

Closes: #9047 - Add Provider Accounts #12057

Merged
merged 8 commits into from
Mar 29, 2023
Merged

Conversation

DanSheps
Copy link
Member

Closes: #9047 - Add Provider Accounts

  • Add Provider Accounts
  • Associate Circuits directly with provider accounts

@DanSheps DanSheps changed the base branch from develop to feature March 24, 2023 19:21
@jeremystretch
Copy link
Member

Hey @DanSheps, thanks for tackling this! I've only given it a quick skim so far, but I have some concerns about the proposed modification to the data model. In its current form, the PR would insert the ProviderAccount model between Circuit and Provider, such that the current relationship of

Circuit.provider --> Provider

becomes

Circuit.provider_account --> ProviderAccount.provider --> Provider

My understanding of the FR was that it would introduce a new model and relationship in parallel to what already exists, with Circuit and ProviderAccount each maintaining a foreign key relationship to Provider. (Please excuse the ASCII art.)

Circuit.provider ------------------------------------------> Provider
Circuit.provider_account --> ProviderAccount.provider --/

IMO this is preferable to the linear implementation, for a few reasons.

  1. The linear relationship introduces a break API change, which is easily avoided with the parallel approach.
  2. It introduces a second degree of relationship that must be traversed to determine a Circuit's Provider.
  3. It forces the user to make utilize the ProviderAccount model, which may be undesirable for some. A parallel approach enables users to leverage the new relationship when desired, without sacrificing any functionality in the interim.
  4. It necessitates the automatic creation of faux models during migration to satisfy the new schema. This is particularly concerning with regard to existing providers that do not specify an account value.

AFAICT, the only downside to the parallel approach is ensuring that a Circuit and its assigned ProviderAccount (if any) both point to the same Provider, but this should be fairly easy to enforce. What do you think?

(FWIW I don't believe this change would require too much modification to the PR, and I'd be happy to assist.)

@ryanmerolle
Copy link
Contributor

ryanmerolle commented Mar 26, 2023

I just tested an here are my 2 cents on what @jeremystretch mentioned. Wrote down my thoughts, but TLDR I think I agree with the points made specifically for 3 & 4.

  1. Breaking API changes can be overlooked if the feature truly nets a positive even with the breaking change. But I get your point.
  2. The second degree relationship does add more work for sure to users, but I view this as the same as breaking changes.
  3. I think this is totally fair, many users will only have 1 account or not even track this sort of thing in NetBox, the parallel approach does allow for more flexibility in this regards.
  4. One way to fix this is to create an "account number" that matches the provider name. Not ideal, I get it.

Whatever way gets decided on here is my feedback on the current implementation:

  • Provider networks do not currently get an account associated. This is likely a FR oversight on my part. 1 option is to not add it since a provider network could have multiple accounts connected. IE the account number would only associate to the circuit. The other option is to add the relationship, and if someone wanted it, they could add a custom relationship.
  • An add circuit button is missing from the account page (may not be needed either since you would more frequently add circuits) - Just pointing it out
  • The REST API for provider account does not show associated circuits.
  • GraphQL looks good

Thanks for the hard work on the PR!

@DanSheps
Copy link
Member Author

DanSheps commented Mar 28, 2023

The REST API for provider account does not show associated circuits.

I don't think we do this anywhere for OneToMany relationships (think interfaces) because the potential query size would be huge and you can't paginate within a JSON response. I might be wrong.

@jeremystretch jeremystretch merged commit 9d709c8 into feature Mar 29, 2023
@jeremystretch jeremystretch deleted the 9047-provideraccounts branch March 29, 2023 12:27
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow more than 1 Account per provider
3 participants