Skip to content

Conversation

@kkthxbye-code
Copy link
Contributor

@kkthxbye-code kkthxbye-code commented May 2, 2022

Fixes: #9280

This PR adds a new boolean field (adopt_components) to the Module form. The creation logic has been changed to check for and change the module field on any existing components.

This is still a draft. I need to verify that the API works, and I haven't checked a few cases. Looking for input for the implementation also.

@kkthxbye-code
Copy link
Contributor Author

A potential issue I found when testing. When you add a module with component names that are already present in on another installed module, which of the following solutions would make most sense?

  • Steal the components. Just changing the module field works fine, but the components will obviously be removed from the existing module.
  • Throw an error (would probably be added to my WIP form validation change here kkthxbye-code@8f765c1)

@kkthxbye-code kkthxbye-code marked this pull request as ready for review May 4, 2022 05:28
@kkthxbye-code
Copy link
Contributor Author

kkthxbye-code commented May 4, 2022

I think this is a good starting point for the feature.

I mentioned that I would like to verify that the API works as expected (it does). However, I can see from the commit here a298187 that disabling replication was specifically only added to the UI. I can bring both replication and adoption into the API also if you want @jeremystretch - not sure if that's another feature request after or if it is even a good idea.

Before merging please test the different scenarios to make sure I didn't miss anything.

When this is merged I'll create a pull request for #9001

@jeremystretch
Copy link
Member

Did some testing with this, works great! 👍

When you add a module with component names that are already present in on another installed module, which of the following solutions would make most sense?

The simplest approach may be to simply ignore any existing components which are already assigned to a module. I'll add a note to my review w/more detail.

I can bring both replication and adoption into the API also if you want @jeremystretch - not sure if that's another feature request after or if it is even a good idea.

Probably worth discussing as a separate issue. For now I think introducing the adoption functionality in the UI only is fine.

@kkthxbye-code
Copy link
Contributor Author

Added prefetching of installed components.

Ignore components with a module set. This will make module installation fail if replicate components is activated. A better error message can be shown when I finish the error message PR.

@kkthxbye-code
Copy link
Contributor Author

I'll fix the test tomorrow.

@jeremystretch jeremystretch merged commit e9bf6a7 into netbox-community:develop May 5, 2022
@kkthxbye-code kkthxbye-code deleted the adopt-module-component branch May 9, 2022 16:36
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 18, 2022
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.

Add an option to adopt existing DeviceComponents when installing modules

2 participants