-
Notifications
You must be signed in to change notification settings - Fork 38
feat: Add SendUserProvidedResourceTags to third party integrations resource #1388
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
base: master
Are you sure you want to change the base?
feat: Add SendUserProvidedResourceTags to third party integrations resource #1388
Conversation
"github.com/mongodb/mongodbatlas-cloudformation-resources/util/progressevent" | ||
"github.com/mongodb/mongodbatlas-cloudformation-resources/util/validator" | ||
"go.mongodb.org/atlas-sdk/v20231115002/admin" | ||
"go.mongodb.org/atlas-sdk/v20250312005/admin" |
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 update is needed in order to implement SendUserProvidedResourceTags
field.
if util.IsStringPresent(currentModel.ServiceDiscovery) && !util.AreStringPtrEqual(currentModel.ServiceDiscovery, integration.ServiceDiscovery) { | ||
integration.ServiceDiscovery = currentModel.ServiceDiscovery | ||
} | ||
if util.IsStringPresent(currentModel.Scheme) && !util.AreStringPtrEqual(currentModel.Scheme, integration.Scheme) { |
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.
scheme
field attribute is removed in more recent versions of the SDK. See this Terraform PR where this same field is removed and is addressed as a breaking change.
…b-atlas-mean-stack-aws-fargate-integration/fargate-example/client (#1381) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Mar Cabrera <[email protected]>
…godb-atlas-mean-stack-aws-fargate-integration/fargate-example/client (#1387) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
#1383) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…18 in /cfn-resources (#1384) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…sources (#1391) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…n-resources (#1390) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…1.61.0 to 1.62.0 in /cfn-resources (#1392) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…0 in /cfn-resources (#1389) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…s-mean-stack-aws-fargate-integration/fargate-example/client (#1398) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
… in /cfn-resources (#1397) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…#1393) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…n-resources (#1400) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Leo Antoli <[email protected]>
…1.62.0 to 1.64.0 in /cfn-resources (#1403) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Leo Antoli <[email protected]>
… SDK version update
…ute not returned by API
Organization contract test is failing, this is not related to this PR |
cfn-resources/third-party-integration/mongodb-atlas-thirdpartyintegration.json
Show resolved
Hide resolved
cfn-resources/third-party-integration/mongodb-atlas-thirdpartyintegration.json
Outdated
Show resolved
Hide resolved
cfn-resources/third-party-integration/test/inputs_1_create.template.json
Outdated
Show resolved
Hide resolved
for i := range integrations.Results { | ||
m := integrationToModel(*currentModel, &integrations.Results[i]) | ||
mm = append(mm, m) | ||
if integrations.HasResults() { |
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.
Same comments as above
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.
Addressed in 881a029
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 just realized, that if I use integrations == nil {}
, here, then when results := integrations.GetResults()
would panic right? Shouldn't it be integrations != nil
instead?
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.
Pull Request Overview
This PR adds the SendUserProvidedResourceTags
field to the third-party integrations resource, allowing users to configure whether user-defined resource tags are included when sending metrics and alerts to third-party services. The implementation includes breaking changes due to an SDK upgrade and removal of the deprecated Scheme
field for Prometheus integrations.
- Adds
SendUserProvidedResourceTags
boolean field to the third-party integration schema - Updates SDK from v20231115002 to v20250312005 (breaking change)
- Removes deprecated
Scheme
field from Prometheus integration configuration
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
mongodb-atlas-thirdpartyintegration.json | Adds new field definition and removes deprecated Scheme field |
cmd/resource/resource.go | Updates SDK usage and implements field handling logic |
cmd/resource/model.go | Updates data model to include new field and remove Scheme |
docs/README.md | Updates documentation to reflect schema changes |
examples/thirdpartyintegrations/datadog.json | Demonstrates usage of new field |
test/*.json | Updates test templates to reflect schema changes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
integrations, resModel, err := client.AtlasSDK.ThirdPartyIntegrationsApi.CreateThirdPartyIntegration(context.Background(), *IntegrationType, *ProjectID, requestBody).Execute() | ||
if err != nil { | ||
if apiError, ok := admin.AsError(err); ok && *apiError.Error == http.StatusConflict { | ||
if apiError, ok := admin.AsError(err); ok && apiError.Error == http.StatusConflict { |
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.
The comparison apiError.Error == http.StatusConflict
is incorrect. apiError.Error
is likely an integer status code, but http.StatusConflict
is also an integer (409). However, the comparison should use *apiError.ErrorCode
or similar field that contains the HTTP status code, not apiError.Error
directly.
if apiError, ok := admin.AsError(err); ok && apiError.Error == http.StatusConflict { | |
if apiError, ok := admin.AsError(err); ok && apiError.ErrorCode == http.StatusConflict { |
Copilot uses AI. Check for mistakes.
"file" | ||
] | ||
}, | ||
"Scheme": { |
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.
While we dont have this standarized in CFN, since we have a breaking change (removal of scheme
) would suggest adding a small changelog file similar to https://github.com/mongodb/mongodbatlas-cloudformation-resources/blob/master/cfn-resources/access-list-api-key/CHANGELOG.md.
"Profile": "default", | ||
"Type": "PROMETHEUS", | ||
"Enabled": true, | ||
"Scheme": "http", |
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.
q: Since we don't have these contract tests running in CI wanted to confirm if you were able to run locally
"TlsPemPath": { | ||
"type": "string", | ||
"description": "Root-relative path to the Transport Layer Security (TLS) Privacy Enhanced Mail (PEM) key and certificate file on the host." | ||
}, |
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.
once merged would consider if you prefer to handle the release as part of a separate ticket, and consider bumping the major version due to the breaking change
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.
Yeah I think that would be a good idea, due to the breaking change. Currently this resource is on 2.0.0
, we can bump to 3.0.0
for the next release.
return progressevent.GetFailedEventByResponse(err.Error(), resModel), nil | ||
} | ||
|
||
if integrations == nil || len(integrations.GetResults()) == 0 { |
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.
if integrations == nil || len(integrations.GetResults()) == 0 { | |
if len(integrations.GetResults()) == 0 { |
no need to check integration is not nil, see GetResults implementation:
func (o *PaginatedIntegration) GetResults() []ThirdPartyIntegration {
if o == nil || IsNil(o.Results) {
var ret []ThirdPartyIntegration
return ret
}
return *o.Results
}
same for the other ocurrence
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.
in fact, as you're using results later, instead of retrieving again, you can do something like:
results := integrations.GetResults()
if len(results) == 0 {
return progressevent.GetFailedEventByResponse("No integration returned from create", resModel), nil
}
return handler.ProgressEvent{
OperationStatus: handler.Success,
ResourceModel: integrationToModel(*currentModel, &results[0]),
}, nil
IntegrationType := currentModel.Type | ||
|
||
_, res, err = client.Atlas20231115002.ThirdPartyIntegrationsApi.DeleteThirdPartyIntegration(context.Background(), *IntegrationType, *ProjectID).Execute() | ||
res, err = client.AtlasSDK.ThirdPartyIntegrationsApi.DeleteThirdPartyIntegration(context.Background(), *IntegrationType, *ProjectID).Execute() |
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.
why define vars res and err separated instead of using := here?
res, err := client.AtlasSDK.ThirdPartyIntegrationsApi.DeleteThirdPartyIntegration(context.Background(), *IntegrationType, *ProjectID).Execute()
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.
Agree to bump this to 3.0.0 @marcabreracast can you bring this up to the weekly sync for awareness?
This PR has gone 30 days without any activity and meets the project’s definition of "stale". This will be auto-closed if there is no new activity over the next 30 days. If the issue is still relevant and active, you can simply comment with a "bump" to keep it open, or add the label "not_stale". Thanks for keeping our repository healthy! |
Proposed changes
Add SendUserProvidedResourceTags field to third-party-integrations resource.
Link to any related issue(s): CLOUDP-327113
Note: This PR includes breaking changes, due to a needed update of the SDK to be able to populate the
SendUserProvidedResourceTags
field. Please see comments below addressing the breaking change.Type of change:
expected)
Manual QA performed:
Required Checklist:
make fmt
and formatted my codeworks in Atlas
Screenshots
Atlas UI after resource creation: