-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Use default labels when deploying service into ECS and allow overwriting existing DB (web) #45180
Conversation
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
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.
any test update?
lib/web/integrations_awsoidc.go
Outdated
databaseAgentMatcherLabels[types.DiscoveryLabelVPCID] = []string{req.VPCID} | ||
databaseAgentMatcherLabels[types.DiscoveryLabelRegion] = []string{req.Region} | ||
databaseAgentMatcherLabels[types.DiscoveryLabelAccountID] = []string{req.AccountID} |
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.
We are not checking this variables.
Does the UI always send them?
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.
thanks for catching this. here is the change
i left the merging of labels as is, just in case we want to bring back custom labels back in the future (probably not likely since we want to keep things as simple as possible), but atm the UI will no longer allow any custom labels
0beb819
to
678f00b
Compare
lib/web/integrations_awsoidc.go
Outdated
if len(req.VPCID) == 0 { | ||
return nil, trace.BadParameter("vpc ID is required") | ||
} | ||
if len(req.Region) == 0 { | ||
return nil, trace.BadParameter("AWS region is required") | ||
} | ||
if len(req.AccountID) == 0 { | ||
return nil, trace.BadParameter("AWS account ID is required") | ||
} |
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.
💅
Checking for empty strings is usually done with == ""
instead of using len
.
I had to double check if those variables were string or lists 😅
678f00b
to
d56d28f
Compare
…ing existing DB (web) (#45180) * Allow overwriting an existing database * Default to pre-defined labels when deploying service into ecs * Address CRs * Reuse create database endpoint to support overwrite Revert database update logic * Add checks for required fields for default labels
part of #41004
required by #45179