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

[minor_change] Add read-only pcTag, scope and segment attributes to aci_bridge_domain resource and datasource and add scope to aci_endpoint_security_group and aci_application_epg resources and datasources (DCNE-257) #1302

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

akinross
Copy link
Collaborator

No description provided.

@akinross akinross added the jira-sync Sync this issue to Jira label Nov 22, 2024
@github-actions github-actions bot changed the title [minor_change] Add read-only pcTag, scope and segment attributes to aci_bridge_domain resource and datasource and add scope to aci_endpoint_security_group and aci_application_epg resources and datasources [minor_change] Add read-only pcTag, scope and segment attributes to aci_bridge_domain resource and datasource and add scope to aci_endpoint_security_group and aci_application_epg resources and datasources (DCNE-257) Nov 22, 2024
@akinross akinross force-pushed the read_only_attribute_updates branch 2 times, most recently from e73c5d6 to d523689 Compare November 22, 2024 10:58
@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 94.53125% with 21 lines in your changes missing coverage. Please review.

Project coverage is 85.49%. Comparing base (7ecea9c) to head (5db7883).

Files with missing lines Patch % Lines
internal/provider/utils.go 0.00% 16 Missing ⚠️
internal/provider/resource_aci_bridge_domain.go 99.09% 3 Missing ⚠️
internal/provider/resource_aci_application_epg.go 88.88% 1 Missing ⚠️
...l/provider/resource_aci_endpoint_security_group.go 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1302      +/-   ##
==========================================
+ Coverage   85.45%   85.49%   +0.04%     
==========================================
  Files         125      125              
  Lines       70689    70986     +297     
==========================================
+ Hits        60409    60693     +284     
- Misses       8324     8337      +13     
  Partials     1956     1956              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -227,6 +227,10 @@ All examples for the Bridge Domain resource can be found in the [examples](https
* `name_alias` (nameAlias) - (string) The name alias of the Bridge Domain object.
* `owner_key` (ownerKey) - (string) The key for enabling clients to own their data for entity correlation.
* `owner_tag` (ownerTag) - (string) A tag for enabling clients to add their own data. For example, to indicate who created this object.
* `pc_tag` (pcTag) - (string) The classification tag used for policy enforcement and zoning.
* `scope` (scope) - (string) The L3 scope ID of the Bridge Domain object.
- Default: `0`
Copy link
Collaborator

@shrsr shrsr Nov 25, 2024

Choose a reason for hiding this comment

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

Shall we skip specifying a Default for this, if it is a Read Only property?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes but it should also not be under optional. We already discussed this issue. Created a new issue to track this: #1303

@@ -1412,14 +1412,9 @@ func (r *{{.ResourceClassName}}Resource) Schema(ctx context.Context, req resourc
stringplanmodifier.UseStateForUnknown(),
},
{{- end}}
{{- if or .ValidValues $.HasReadOnlyProperties}}
{{- if or .ValidValues}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't this change bring the "update in place" issue we spoke about earlier? or was this fixed in Terraform side?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do not see this issues so not sure this is the same situation you had.

The “MakeSingleNestedAttributeRequiredAttributesNotProvidedValidator("{{.ResourceName}}", []string{"{{listToString .RequiredPropertiesNames}}"}),” part already handles the required setting in single nested attributes to allow for {} to be passed in when there are required attributes so the MakeStringRequired should not be used in the validator.

@akinross akinross requested a review from shrsr November 25, 2024 22:47
sajagana
sajagana previously approved these changes Nov 29, 2024
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!

shrsr
shrsr previously approved these changes Nov 29, 2024
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

samiib
samiib previously approved these changes Dec 3, 2024
Copy link
Collaborator

@samiib samiib left a comment

Choose a reason for hiding this comment

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

LGTM

@akinross akinross dismissed stale reviews from samiib, shrsr, and sajagana via 85a8767 December 19, 2024 09:06
@akinross akinross force-pushed the read_only_attribute_updates branch from d523689 to 85a8767 Compare December 19, 2024 09:06
@akinross akinross force-pushed the read_only_attribute_updates branch from 85a8767 to f93e021 Compare December 20, 2024 07:54
…ci_bridge_domain resource and datasource and add scope to aci_endpoint_security_group and aci_application_epg resources and datasources
@akinross akinross force-pushed the read_only_attribute_updates branch from f93e021 to 8f7cd5c Compare December 20, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira-sync Sync this issue to Jira
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants