-
Notifications
You must be signed in to change notification settings - Fork 166
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
fix: project delete function uses correct string representation of id #1733
Conversation
@@ -269,7 +269,7 @@ func (r *projectRS) Create(ctx context.Context, req resource.CreateRequest, resp | |||
|
|||
_, _, err := connV2.TeamsApi.AddAllTeamsToProject(ctx, project.GetId(), NewTeamRoleList(ctx, teams)).Execute() | |||
if err != nil { | |||
errd := deleteProject(ctx, r.Client.AtlasV2, project.Id) | |||
errd := deleteProject(ctx, r.Client.AtlasV2, project.GetId()) |
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.
can we have a test that fails before this change and passes after?
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.
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.
great, thx!
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, one change to consider
dependents := AtlasProjectDependants{AdvancedClusters: clusters} | ||
|
||
if _, ok := admin.AsError(err); ok { | ||
return nil, "", err | ||
if _, ok := admin.AsError(listClustersErr); ok { |
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 would imply removing redudant if statement below)
if _, ok := admin.AsError(listClustersErr); ok { | |
if listClustersErr != nil { |
curious if we want to unify both error checks, don't see a reason why for ApiError we would return error, while for others SDK error types keep on retrying. Would simply return error for an encountered error.
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 didn't want to change this to avoid changing behaviour, but I think it makes sense to unify it.
|
* master: fix: project delete function uses correct string representation of id (#1733)
Description
Project delete was using wrong string representation of Id, with quotes (
"projectId"
) instead of plain string (projectId
).Within our project delete function we have a logic that waits for all "dependants" to be deleted.
During the SDK migration there was a change with how to obtain the project id that is used to list the cluster of a project, which is now passing an incorrect value. This API call uses nonNullProjectID of type types.String, so nonNullProjectID.String() was responding with "6575af27f93c7a6a4b50b239" resulting in an error by the SDK: "groupId must have less than 24 elements”. Due to whats seems like incorrect logic, this error simply lead to retrying constantly until eventually the timeout is reached.
Type of change:
Required Checklist:
Further comments