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

member_count in nested virtual chassis should be removed from example model in REST API documentation. #9592

Closed
PaulWestphal opened this issue Jun 23, 2022 · 7 comments
Assignees
Labels
status: accepted This issue has been accepted for implementation type: documentation A change or addition to the documentation

Comments

@PaulWestphal
Copy link

Change Type

Correction

Area

REST API

Proposed Changes

As explained in this issue (#9329), the nested virtual chassis from the /dcim/devices/ endpoint does not return a member_count. However, the swagger-ui documentation clearly shows an example model WITH this field.

Jeremy argued that this field should not exist, so it should be removed from the example model as well.

@PaulWestphal PaulWestphal added the type: documentation A change or addition to the documentation label Jun 23, 2022
@PaulWestphal PaulWestphal changed the title member_count in nested virtual chassis should be removed from example in REST API documentation. member_count in nested virtual chassis should be removed from example model in REST API documentation. Jun 23, 2022
@jeremystretch jeremystretch added the status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation label Jun 23, 2022
@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. NetBox is governed by a small group of core maintainers which means not all opened issues may receive direct feedback. Do not attempt to circumvent this process by "bumping" the issue; doing so will result in its immediate closure and you may be barred from participating in any future discussions. Please see our contributing guide.

@github-actions github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Aug 23, 2022
@amhn
Copy link
Contributor

amhn commented Aug 28, 2022

member_count needs to be removed from NestedVirtualChassisSerializer and the corresponding test.

Happy to provide a PR.

@DanSheps DanSheps added status: accepted This issue has been accepted for implementation and removed status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation pending closure Requires immediate attention to avoid being closed for inactivity labels Aug 29, 2022
@DanSheps
Copy link
Member

@amhn I have assigned this to you.

@jeremystretch
Copy link
Member

member_count needs to be removed from NestedVirtualChassisSerializer

This is not quite correct. The member_count field is valid and necessary under the /api/dcim/virtual-chassis/, and we should not remove it. The issue identified under #9329 is its erroneous inclusion under other endpoints (e.g. /api/dcim/devices/) where the necessary data is not available. Solving for this requires the introduction of a second nested serializer for use in these cases.

However, we can work around this by instead caching the count of members on each VirtualChassis instance per #10197. As this approach avoids any potentially disruptive changes to the REST API while also resolving the root issue, I'm going to close this issue in favor of the caching approach.

@jeremystretch jeremystretch closed this as not planned Won't fix, can't repro, duplicate, stale Aug 29, 2022
@amhn
Copy link
Contributor

amhn commented Aug 29, 2022

@jeremystretch Providing the the member_count is surely the better solution and should be favored.

But as far as I understood what you describe is exactly what I implemented. The Endpoints in /api/dcim/virtual_chassis still provide member_count and it is only removed from the ones under /api/dcim/devices. Those are as far as I can see the only ones using NestedVirtualChassisSerializer.

I don't want to argue with you. Your solution of caching the member_count is surely the better solution for users of the API. Just want to learn what I misunderstood.

@jeremystretch
Copy link
Member

Those are as far as I can see the only ones using NestedVirtualChassisSerializer.

As with all objects, the nested serializer is also used when using "brief" mode for the model endpoint (i.e. /api/dcim/virtual-chassis/?brief=true), so the field needs to stay.

@amhn
Copy link
Contributor

amhn commented Aug 29, 2022

Thank you for the clarification. That is what I suspected, but could not verify. Turns out I was using the wrong netbox instance.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation type: documentation A change or addition to the documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants