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

Fmc device bgp + bfd + query_parameter #213

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

Fmc device bgp + bfd + query_parameter #213

wants to merge 33 commits into from

Conversation

mmaciejc
Copy link
Contributor

Fixes in schema - adding computed, tf_name changes

Change in template:
added query_parameter support

Autogenerated:
fmc_device_bgp_genreal_settings
fmc_device_bgp
fmc_bfd_template
fmc_device_bfd

fmc_device_ha_pair_monitoring

  • changes in resource delete function - delete by PUT

@mmaciejc mmaciejc requested a review from danischm December 13, 2024 11:35
@@ -7,7 +7,8 @@ rest_endpoint: str(required=False) # REST endpoint path
put_create: bool(required=False) # Set to true if the PUT request is used for create
no_update: bool(required=False) # Set to true if the PUT request is not supported
no_delete: bool(required=False) # Set to true if the DELETE request is not supported
data_source_name_query: bool(required=False) # Set to true if the data source supports name queries
data_source_name_query: bool(required=False) # Set to true if the data source supports name queries ### Equal to "query_parameter: name"
Copy link
Member

Choose a reason for hiding this comment

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

Let's not have two ways to achieve the same thing. If we introduce a more flexible option which makes this option obsolete, let's remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is the goal, however I wanted to take a step in between, where we support both to make sure there is an agreement on how this should be implemented (before re-doing all YAML definitions).

Copy link
Contributor

Choose a reason for hiding this comment

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

Implemented for all resources

@@ -7,7 +7,8 @@ rest_endpoint: str(required=False) # REST endpoint path
put_create: bool(required=False) # Set to true if the PUT request is used for create
no_update: bool(required=False) # Set to true if the PUT request is not supported
no_delete: bool(required=False) # Set to true if the DELETE request is not supported
data_source_name_query: bool(required=False) # Set to true if the data source supports name queries
data_source_name_query: bool(required=False) # Set to true if the data source supports name queries ### Equal to "query_parameter: name"
query_parameter: str(required=False) # Additional parameter (tf_name), in addition to `id`, to search for object in `data source` and `put create`
Copy link
Member

Choose a reason for hiding this comment

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

In other implementations query_parameter is referring to a query param in the URL. I would suggestion to use something like data_source_query_attribute to be more specific.

Also, would it make sense to define this at the attribute instead of definition level? Which would possibly also simplify the implementation (templates). Something like:

attributes:
  - model_name: ifname
    data_path: [interface]
    tf_name: interface_logical_name
    description: Logical Name of the interface of BFD assignment if SINGLE_HOP selected.
    type: String
    data_source_query: true
    example: "outside"

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, let's rename it to data_source_query_attribute.
I believe making this global, makes it easy to find. Also putting it under attribute as true value, could suggest that we can have multiple occurrences of those, which is not our intention at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Implemented under attribute as suggested

@mmaciejc mmaciejc added the CORE Changes in core files of provider or generator. label Dec 18, 2024
@rchrabas rchrabas requested a review from danischm December 18, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CORE Changes in core files of provider or generator.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants