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

fix: object_handling method for overriding defaults #589

Merged
merged 2 commits into from
Nov 14, 2024

Conversation

alperenkose
Copy link
Collaborator

Description

This is an alternative approach for mitigating issues on passing None value to sdk for params which have default values in order to remove an xpath.

#587 PR and relevant pan-os-python changes would not be needed if we prefer this approach.

Motivation and Context

We are using default values for None provided params for newly created/replaced objects which can cause issue when SDK requires an actual None type input to remove an xpath.

For example pan-os-python does not accept 'none' string value for the nexthop type in StaticRoute but it requires nexthop type to be None type in order to be removed (in order words set to "None" in GUI), although it has a default value of 'ip-address'. Normally we expect to use default values from SDK when the param is None type but this kind of cases breaks this approach.

With the proposed object_handling() method, it is possible to override values after defaults are assigned so we can mitigate this kind of issues.

Also fixes #584

How Has This Been Tested?

Tested with manually written ansible playbooks.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes if appropriate.
  • All new and existing tests passed.

This change also requires an update on pan-os-python to accept 'none'
value for the nexthop type.
Allows overriding values for newly created/replaced objects
for mitigating issues on passing `None` value to sdk for params
with default values.

With this change pan-os-python does not need to accept 'none' value
for the nexthop type in panos_static_route.
@adambaumeister
Copy link
Collaborator

So to confirm my understanding, the issue here is that some objects in pan-os support being a literal "none" value, but right now we can't pass literal "None" from the modules as the default overrides it. This causes an issue in the circumstance when you want to set the type as "none" because we're always passing it a default. It it right?

@alperenkose
Copy link
Collaborator Author

The issue is pan-os-python expects a None type value being passed for some parameters when it should be removed from xpath. However in the current situation when a value is None type for non-existing objects the default value is being used for the param (e.g. when nexthop_type is None type it's converted to ip-address because of the default value for this parameter, however the intention was to remove/skip nexthop_type in the xpath)
This PR allows default values to be altered with an object_handling() method which can be overridden on specific modules if needed.

@alperenkose alperenkose marked this pull request as ready for review November 14, 2024 08:23
Copy link
Collaborator

@adambaumeister adambaumeister left a comment

Choose a reason for hiding this comment

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

lgtm

plugins/modules/panos_static_route.py Show resolved Hide resolved
@alperenkose alperenkose merged commit 2a085cb into develop Nov 14, 2024
24 checks passed
github-actions bot pushed a commit that referenced this pull request Nov 14, 2024
### [2.21.3](v2.21.2...v2.21.3) (2024-11-14)

### Bug Fixes

* Add new community argument for bgp_policy_rule ([#543](#543)) ([255ebed](255ebed))
* implement object_handling method for overriding defaults ([#589](#589)) ([2a085cb](2a085cb))
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.

paloaltonetworks.panos.panos_static_route on 2.21.2
2 participants