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 Netbox v4.1.* compatibility #2

Closed
wants to merge 1 commit into from

Conversation

fanshan
Copy link
Contributor

@fanshan fanshan commented Sep 25, 2024

Hello,

Thank you for developing this plugin. We would like to tests it, but we use Netbox v4.1.* versions.

For that, I propose a little patch I think is not complete. I have noted these last points to see with you in order to finalize this PR:

  • Review the NetBox changelog for any relevant code changes
  • Run automated tests and resolve any errors
  • Perform manual testing of UI elements
  • Perform manual testing of NetBox branching

If you see any other point to raise, I can update this PR as well.

@pheus
Copy link
Owner

pheus commented Sep 26, 2024

Hello @fanshan,

Thank you for your interest in this plugin and for opening the PR. You're correct that this patch removes the safeguards preventing the plugin from running against NetBox 4.1, but it will require both automated and manual testing before I can accept these changes.

For this purpose, I recommend reordering and expanding the task list as follows:

  • Review the NetBox changelog for any relevant code changes
  • Run automated tests and resolve any errors
  • Perform manual testing of UI elements
  • Perform manual testing of NetBox branching
  • Update plugin dependencies
  • Update NetBox version requirements
  • Update documentation
  • Release a new version

Would you be interested in helping with testing and fixing any upgrade-related errors/bugs? In my initial testing, I encountered several GraphQL-related errors.

Otherwise, I can aim to provide a compatible version within the next few days.

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@fanshan
Copy link
Contributor Author

fanshan commented Sep 27, 2024

Hello @pheus,

I can try to fix errors raised by automated tests and update the plugin dependencies.

Another question: do you want to keep compatibility with Netbox version inferior to v4.1.0?

@pheus
Copy link
Owner

pheus commented Sep 27, 2024

Thank you very much for your help!

Another question: do you want to keep compatibility with Netbox version inferior to v4.1.0?

At this stage of development, it is fine to raise the minimum required version to 4.1.0.

@fanshan
Copy link
Contributor Author

fanshan commented Oct 1, 2024

Hello @pheus,

I still have this kind of error when I run the tests:

======================================================================
FAIL: test_graphql_get_object (netbox_aci_plugin.tests.test_api.ACIVRFAPIViewTestCase.test_graphql_get_object)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Volumes/workspace/projects/saashup/netbox/v4/venv/lib/python3.12/site-packages/django/test/utils.py", line 443, in inner
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/Volumes/workspace/projects/saashup/netbox/v4/netbox/netbox/utilities/testing/api.py", line 555, in test_graphql_get_object
    self.assertIn('errors', data)
AssertionError: 'errors' not found in {'data': {'aci_vrf': {'changelog': [], 'display': 'ACIVRFTestAPI1', 'class_type': 'ACIVRF', 'tags': [], 'journal_entries': [], 'custom_fields': {}, 'aci_tenant': {'id': '43'}, 'nb_tenant': {'id': '29'}, 'nb_vrf': {'id': '12'}, 'dns_labels': ['DNS1', 'DNS2'], 'id': '28', 'created': '2024-10-01T11:59:27.402923+00:00', 'last_updated': '2024-10-01T11:59:27.402926+00:00', 'custom_field_data': {}, 'name': 'ACIVRFTestAPI1', 'name_alias': 'Testing', 'description': 'First ACI Test', 'bd_enforcement_enabled': False, 'ip_data_plane_learning_enabled': True, 'pc_enforcement_direction': 'ingress', 'pc_enforcement_preference': 'unenforced', 'pim_ipv4_enabled': False, 'pim_ipv6_enabled': False, 'preferred_group_enabled': False, 'comments': '# ACI Test 1'}}}

Does this error tell you something?

@pheus
Copy link
Owner

pheus commented Oct 1, 2024

Hi @fanshan!

Thank you for getting this far!

I haven't encountered this error before. From my understanding of the test case in NetBox, it seems to be testing an invalid permission setting, which should return an empty query result with an error code or message.

In your case, the GraphQL query returns the object without any errors, which is unexpected.

You can find the relevant NetBox code here:
https://github.com/netbox-community/netbox/blob/d9028f92022e3169c0ef138efca7d9e03e38b484/netbox/utilities/testing/api.py#L520

Would you mind sharing your current code?
You can mark this PR as a draft in the meantime.

@pheus
Copy link
Owner

pheus commented Oct 1, 2024

Hi @fanshan,

I was able to narrow down the problem. It's actually related to GraphQL changes introduced in v4.0.10 and v4.0.11.

Here are the relevant PRs:
netbox-community/netbox#17244
netbox-community/netbox#17312

I will update the code to ensure compatibility with v4.0.11.

@pheus
Copy link
Owner

pheus commented Oct 1, 2024

Hi @fanshan,

It seems that with these changes, there are no more errors with NetBox versions 4.1.0, 4.1.1, and 4.1.2.
I hope you didn’t invest too much time troubleshooting the tests. I actually wanted to support you in finding the root cause, but I had to fix the tests myself to reproduce the error.

If you're still interested, you can update your fork and rebase your changes on top. Afterward, I would be happy to accept your PR.

Nonetheless, I really want to thank you for your time and effort.

@fanshan
Copy link
Contributor Author

fanshan commented Oct 2, 2024

Hello @pheus,

It's was fine ! Tests pass and the plugin is improved :-)

Branch rebased. Let me know if you want to change the plugin version.

@pheus
Copy link
Owner

pheus commented Oct 2, 2024

Hello @fanshan,

Thank you very much for this great work! 🚀
I will merge your PR and take it from here.

Copy link
Owner

@pheus pheus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done! Thank you very much 🥇

@pheus pheus closed this in fedac1a Oct 2, 2024
@fanshan fanshan deleted the netbox_v41_compatibility branch October 3, 2024 08:07
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