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

Generated resource and datasource for fvSiteAssociated and fvRemoteId #1233

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

Conversation

abrahammughal
Copy link
Collaborator

Fixes #1224

@akinross
Copy link
Collaborator

akinross commented Jun 14, 2024

CI failing on go generate, make sure you run go generate before you push the changes. This ensures that you have not made any manual changes that the go generate command will wipe away.

Please use [ignore] tag for commits that are not related text that need to be in release notes / changes. So Generated resource and datasource for fvSiteAssociated would be a minor_change.
Created fvRemoteId class would be a minor_change. The other would be ignore.

internal/provider/test_constants.go Outdated Show resolved Hide resolved
@abrahammughal abrahammughal force-pushed the associated_site_and_remote_id branch 3 times, most recently from 6ba7399 to 279aee2 Compare June 25, 2024 22:59
@abrahammughal abrahammughal force-pushed the associated_site_and_remote_id branch 2 times, most recently from d46c36f to bed4ce3 Compare July 10, 2024 22:46
@codecov-commenter
Copy link

codecov-commenter commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 86.19173% with 157 lines in your changes missing coverage. Please review.

Project coverage is 84.18%. Comparing base (e45252b) to head (5b6255c).
Report is 9 commits behind head on master.

Files Patch % Lines
internal/provider/resource_aci_associated_site.go 83.94% 45 Missing and 25 partials ⚠️
...l/provider/resource_aci_remote_site_id_mappings.go 84.91% 45 Missing and 25 partials ⚠️
...ternal/provider/data_source_aci_associated_site.go 92.85% 6 Missing and 2 partials ⚠️
...rovider/data_source_aci_remote_site_id_mappings.go 93.33% 6 Missing and 2 partials ⚠️
internal/provider/utils.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1233      +/-   ##
==========================================
+ Coverage   84.05%   84.18%   +0.13%     
==========================================
  Files          61       65       +4     
  Lines       17940    19076    +1136     
==========================================
+ Hits        15080    16060     +980     
- Misses       2054     2156     +102     
- Partials      806      860      +54     

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

docs/resources/associated_site.md Show resolved Hide resolved
docs/resources/remote_site_id_mappings.md Show resolved Hide resolved
docs/resources/associated_site.md Outdated Show resolved Hide resolved
gen/definitions/classes.yaml Outdated Show resolved Hide resolved
gen/definitions/classes.yaml Show resolved Hide resolved
gen/templates/datasource_test.go.tmpl Outdated Show resolved Hide resolved
gen/templates/datasource_test.go.tmpl Show resolved Hide resolved
internal/provider/test_constants.go Outdated Show resolved Hide resolved
internal/provider/test_constants.go Outdated Show resolved Hide resolved
internal/provider/utils.go Outdated Show resolved Hide resolved
@akinross
Copy link
Collaborator

tests are failing, please check that all test are working

Comment on lines +1361 to +1386
if propertyName == "remoteCtxPcTag" || propertyName == "remotePcTag" {
validValue = "defaultValue"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, we shouldn't hard code property behaviour in the generator. I suggest we provide a way to configure this in the overwrite logic instead of a constant.

gen/generator.go Outdated
Comment on lines 1666 to 1680
// precedenceList := []string{classPkgName, "global"}
// for _, precedence := range precedenceList {
// if classDetails, ok := definitions.Properties[precedence]; ok {
// for key, value := range classDetails.(map[interface{}]interface{}) {
// if key.(string) == "resource_required" {
// for _, v := range value.([]interface{}) {
// if v.(string) == propertyName {
// return true
// }
// }
// }
// }
// }
// }
// return false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes please, commented out code should be removed.

* `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.
* `site_id` (siteId) - (string) A number between 0 and 1000 to identify the primary site being associated.
- Default: `0.000000`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the default be a decimal value or just a whole number, eg 0?


#### Required ####

* `key` (key) - (string) The key used to uniquely identify this configuration object.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this configuration object -> Associated Site object.

the description for value is not clear.

Comment on lines +148 to +150
* `key` (key) - (string) The key used to uniquely identify this configuration object.
* `value` (value) - (string) The value of the property.
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as before

Comment on lines +114 to +115
* `key` (key) - (string) The key used to uniquely identify this configuration object.
* `value` (value) - (string) The value of the property.
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as before

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.

Add resource and datasource for fvSiteAssociated and fvRemoteId (DCNE-190)
6 participants