fix issue 1479 - netbox_service - More than one result returned for

[cernymi]

Related Issue

Issue #1479

(and PR #1427 seems missing ALLOWED_QUERY_PARAMS part)

Modified Behavior

• add parent_object_type and parent_object_id to services ALLOWED_QUERY_PARAMS
• add workaround to _build_query_params for services and Netbox 4.3.0 - 4.4.3
  ipam/services: GET parent_object_type - wrong data type - integer netbox#20554

New Behavior

None

...

Contrast to Current Behavior

You'll be able to create more services with the same name but different parent. For netbox 4.4.4 and never
Parent param introduced in Netbox 4.3.0

...

Discussion: Benefits and Drawbacks

Services will work correctly again for Netbox 4.4.4 (4.3.0) and never

• parent_object_type and parent_object_id should be added to services ALLOWED_QUERY_PARAMS
• be able to query services correctly. Only name of service is not globally unique for Netbox 4.3.0 and never. So parent parameters should be involved.

...

Changes to the Documentation

None

...

Proposed Release Note Entry

• add parent_object_type and parent_object_id to services ALLOWED_QUERY_PARAMS

...

Double Check

• I have read the comments and followed the CONTRIBUTING.md.
• I have explained my PR according to the information in the comments or in a linked issue.
• My PR targets the devel branch.

[sc68cal]

I have this on my list to take a look at, sorry for the delay

[sc68cal]

So, my main problem with this PR is that there are a couple changes in this PR. They are valid changes and good to have, but I would feel more comfortable if we broke them apart so that if there was a problem, we could easily revert. If that is even possible? I do understand why they are all part of this PR but changes to how we parse the version string from the Netbox API response is always a little fraught. We recently got bitten by a change that the Docker deployment of NetBox does where they append their release version to the string, so that is why I am a little shy about this PR.

If this is indeed an all or nothing change, I would appreciate some unit tests around version string parsing just to give me peace of mind.

Thanks, and great work.

[sc68cal]

Just remembered that the CI tests run against live Netbox so the version string does get parsed and tested in CI. Sorry, brain fart. I think this is good.

[sc68cal]

Just needs yamllint fix to some change of fragments

[cernymi]

Thank you for your comments. They're helping me to get familiar with this project. You’re doing a great job!
And yes, you're right. This PR contains more topics than there should be. I completely agree.
Let's plit it to more PR. There's no problem.

• yamllint - PR - 1490

• sanitize full_version and improve _version_check_greater
  going to create PR

• In this PR will be kept:

  • _version_less_than - I'll add unit tests
  • ALLOWED_QUERY_PARAMS - add parent_object_type, parent_object_id
  • workaround for services in _build_query_params

• optionally I'll send another PR for rename version variables:

  • version -> api_version
  • full_version -> netbox_version
    The new one might be less confusing

[sc68cal]

Excellent idea. I've approved and merged the changes for yamllint, so thank you for that. If you rebase this PR on the latest from devel that should get this PR to pass CI

[cernymi]

Alright, thanks. I'll rebase and modify this PR afterwards.
Before that, I've created PR 1491 as a prerequisite.
Let me know if anything needs improvement.

[sc68cal]

merged 1491

[cernymi]

Tested duplicate service names - Result

NetBox version	Status
Community v4.2.9	✅
Community v4.4.4	✅
Community v4.3.7‑Docker‑3.3.0 (covers 4.3.0‑4.4.3)	⚠️ Only one service with a given name can exist

Take‑away:

• Netbox 4.3.0 < version >= 4.4.4 allow multiple services using the same name.
• Version 4.3.0‑4.4.3 behaves incorrectly.

[cernymi]

I still struggle with:

  
ERROR! Error when finding available api versions from default (https://galaxy.ansible.com/) (HTTP Code: 500, Message: Internal Server Error)
Starting galaxy collection install process
Process install dependency map
Error: Process completed with exit code 1.

I'll try push later on to have the pipeline green

[cernymi]

Great, pipeline is green. I have nothing to add.
Let me know if anything needs improvement. Thanks

[sc68cal]

Great job, thank you for breaking this into different PRs, this made things far easier to review.

[cernymi]

Yeah, breaking the original work into multiple PRs definitely made the whole process smoother.
Thanks for taking the time to review them and for the useful feedback — it helped tighten things up a lot.