-
Notifications
You must be signed in to change notification settings - Fork 32
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 l3out schema, template and on_apic attributes (DCNE-161) #291
Conversation
"l3out_template": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Optional: true, | ||
Computed: true, | ||
ValidateFunc: validation.StringLenBetween(1, 1000), | ||
}, | ||
"l3out_schema": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Optional: true, | ||
Computed: true, | ||
ValidateFunc: validation.StringLenBetween(1, 1000), | ||
}, | ||
"l3out_on_apic": &schema.Schema{ | ||
Type: schema.TypeBool, | ||
Optional: true, | ||
Default: false, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- handle the read and import of these attributes.
- adjust the datasource for these attributes.
- update the example to include a APIC and a NDO l3out
- update documentation with attributes missing ( website/docs/r/mso_schema_site_external_epg.html.markdown )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juchowan Can you please update the following:
- data source of schema_site_external_epg with the attributes you introduced in the resource
- documentation of the data source schema_site_external_epg with new attributes
Please let us know if you have any questions.
@shrsr the datasource already exposes these attributes and the are also present in the docs. The boolean value has no value I would say in the data-source as the l3out_dn is important from that perspective only. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
l3outRefMap["schemaId"] = l3outSchema | ||
l3outRefMap["templateName"] = l3outTemplate | ||
l3outRefMap["l3outName"] = l3outName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking we still miss a thing. We can do two things actually.
- We should do a check that the template and schema are provided because they are optional but I believe they are required for the ref to be constructed.
- Or we can set the schema/template of the external epg when these are not provided. ( my preference )
Another thing is should we add validation of config to make l3outSchema/l3outTemplate mutually exclusive from l3out_on_apic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This behaviour should also be described in documentation.
"l3out_on_apic": &schema.Schema{ | ||
Type: schema.TypeBool, | ||
Optional: true, | ||
Default: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a default here? Is not providing it not the same as false. In documentation we should mention the default.
…efaults, add validation for mutually adds logic for computing l3out schema and tempalte if not provided, removes default l3out_on_apic adds validation for mutually exclusive attributes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
DCNE-161