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

198 contract directives #238

Merged
merged 8 commits into from
Oct 5, 2023

Conversation

akinross
Copy link
Collaborator

fixes #198

@akinross akinross force-pushed the 198_contract_directives branch from 841fda9 to ef5465a Compare August 25, 2023 11:08
@akinross akinross requested a review from sajagana August 25, 2023 11:18
@akinross akinross requested a review from sajagana August 28, 2023 10:35
sajagana
sajagana previously approved these changes Aug 28, 2023
Copy link
Collaborator

@sajagana sajagana left a comment

Choose a reason for hiding this comment

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

LGTM!

filter_type = "bothWay"
action = "deny"
priority = "level2"
directives = ["log", ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

directives = ["log", ] -> directives = ["log"]

mso/resource_mso_schema_template_contract.go Show resolved Hide resolved
examples/schema_template_contract/main.tf Show resolved Hide resolved
* `filter_schema_id` - (Read-Only) The schema ID of the Filter.
* `filter_template_name` - (Read-Only) The template name of the Filter.
* `filter_name` - (Read-Only) The name of the Filter.
* `directives` - **Deprecated** (Read-Only) The directives of the Filter.
Copy link
Collaborator

Choose a reason for hiding this comment

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

filter_relationships and directives -> -(Read-Only) Deprecated

* `filter_relationship.filter_name` - (Required) The filter to associate with this contract.

* `directives` - (Required) A list of filter directives. Allowed values are `log` and `none`.
* `directives` - **Deprecated** (Optional) A list of filter directives. Allowed values are `none`, `no_stats`, and `log`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Deprecated (Optional) -> could we use any one of the format in consistance?

Copy link
Collaborator

@shrsr shrsr left a comment

Choose a reason for hiding this comment

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

LGTM

anvitha-jain
anvitha-jain previously approved these changes Sep 8, 2023
@shrsr shrsr self-requested a review September 11, 2023 20:30
shrsr
shrsr previously approved these changes Sep 11, 2023
Copy link
Collaborator

@shrsr shrsr left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 48 to 60
* `filter_relationship` - (Optional) A list of Filter Relationships for the Contract.
* `filter_relationship.filter_name` - (Required) The name of the Filter associated with the Contract.
* `filter_relationship.filter_schema_id` - (Optional) The schema ID of the Filter associated with the Contract.
* `filter_relationship.filter_template_name` - (Optional) The template name of the Filter associated with the Contract.
* `filter_relationship.filter_type` - (Optional) The type of the Filter associated with the Contract. Allowed values are `bothWay`, `consumer_to_provider` and `provider_to_consumer`. Defaults to `bothWay`.
* `filter_relationship.directives` - (Optional) A list of filter directives associated with the Contract. Allowed values are `none`, `no_stats`, and `log`.
* `filter_relationship.action` - (Optional) The action of the Filter associated with the Contract. Allowed values are `deny` and `permit`.
* `filter_relationship.priority` - (Optional) The override priority of the Filter associated with the Contract. Allowed values are `default`, `level1`, `level2`, and `level3`.

* `filter_relationships` - (Optional) **Deprecated** A Map to provide one Filter Relationship. This attribute is deprecated, use `filter_relationship` instead. It is not allowed to use in combination with `filter_relationship`.
* `filter_relationships.filter_schema_id` - (Optional) The schemaId in which the filter is located.
* `filter_relationships.filter_template_name` - (Optional) The template name in which the filter is located.
* `filter_relationships.filter_name` - (Required) The filter to associate with this contract.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please fix the document formatting to match with data source and also add the type for the filter_relationship

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

filter_type is already added for filter_relationship

mso/resource_mso_schema_template_contract.go Show resolved Hide resolved
mso/resource_mso_schema_template_contract.go Show resolved Hide resolved
sajagana
sajagana previously approved these changes Sep 14, 2023
Copy link
Collaborator

@sajagana sajagana left a comment

Choose a reason for hiding this comment

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

LGTM!

anvitha-jain
anvitha-jain previously approved these changes Sep 15, 2023
shrsr
shrsr previously approved these changes Sep 15, 2023
Copy link
Collaborator

@shrsr shrsr left a comment

Choose a reason for hiding this comment

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

LGTM

},
},
"filter_relationships": {
Type: schema.TypeMap,
Copy link
Member

Choose a reason for hiding this comment

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

This need to be TypeList, aren't you mixing up filter_relationships and filter_relationship in the data source vs the resource?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No we had the discussion regarding this. I originally got confused by this and introduced a change in (fdb2c1e) but that was wrong change. Occurred due to the confusing naming. What I should have done back then was introduce the filter_relationship type list attribute in the datasource, to be consistent with the resource. This change is the revert of that commit with the new attributes added to the filter_relationship (list type).

"oneWay",
}, false),
},
"filter_relationship": {
Copy link
Member

Choose a reason for hiding this comment

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

Why did we flip filter_relationship and filter_relationships order in schema? This breaks the diff calculation quite a lot.

Comment on lines +38 to +50
* `filter_relationship` - (Read-Only) A List of Filter relationships.
* `filter_schema_id` - (Read-Only) The schema ID of the Filter.
* `filter_template_name` - (Read-Only) The template name of the Filter.
* `filter_name` - (Read-Only) The name of the Filter.
* `filter_type` - (Read-Only) The type of the Filter.
* `action` - (Read-Only) The action of the Filter.
* `directives` - (Read-Only) The directives of the Filter.
* `priority` - (Read-Only) The priority override of the Filter.

* `filter_relationships` - (Read-Only) **Deprecated** A map of the Filter relationship.
* `filter_schema_id` - (Read-Only) The schema ID of the Filter.
* `filter_template_name` - (Read-Only) The template name of the Filter.
* `filter_name` - (Read-Only) The name of the Filter.
Copy link
Member

Choose a reason for hiding this comment

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

Are we removing attributes from the existing filter_relationships?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No filter_relationships is the original map that contained 3 attributes schema_id/template_name/name. This is confusing because the naming was wrong. Where originally there was filter_relationships which was a map, then filter_relationship was added after for the list form (e718007). So we are not removing we are just not providing any new attributes to the filter_relationships map, since it is deprecated.

@akinross akinross requested a review from lhercot October 5, 2023 19:02
…ority to filter_relationship attribute and add target_dscp and priority attributes to mso_schema_template_conract and add input validation for attributes and align datasource with resource
…ix idempotency for order of directives in filter relationship for mso_schema_template_contract
@akinross akinross dismissed stale reviews from shrsr, anvitha-jain, and sajagana via 0f6f050 October 5, 2023 20:03
@akinross akinross force-pushed the 198_contract_directives branch from 8ba8fa4 to 0f6f050 Compare October 5, 2023 20:03
Copy link
Member

@lhercot lhercot left a comment

Choose a reason for hiding this comment

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

LGTM

@lhercot lhercot merged commit cec2049 into CiscoDevNet:master Oct 5, 2023
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.

directives attribute should be removed from mso_schema_template_contract resource and data source
5 participants