chore: Uses new Atlas Go SDK version #1831

lantoli · 2024-01-12T10:07:16Z

Description

Uses new Atlas Go SDK version.

The goal of this PR is to do as few changes as possible while keeping the same behavior as the current code.
Next PRs will improve update operation, createonly and readonly attributes of each TF resource using this SDK.

Link to any related issue(s): https://jira.mongodb.org/browse/CLOUDP-222424

Type of change:

Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR.
Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR.
This change requires a documentation update
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the contribution guidelines
I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
I have added any necessary documentation (if appropriate)
I have run make fmt and formatted my code
If changes include deprecations or removals, I defined an isolated PR with a relevant title as it will be used in the auto-generated changelog.

Further comments

internal/common/conversion/type_conversion.go

…(govet)

…un tests" This reverts commit 404512e.

internal/common/conversion/type_conversion.go

internal/common/conversion/type_conversion_test.go

internal/service/atlasuser/data_source_atlas_users_test.go

internal/service/alertconfiguration/data_source_alert_configuration.go

gssbzn · 2024-01-15T09:50:28Z

internal/service/databaseuser/model_database_user.go

+		Roles:        conversion.NonEmptySliceToSlicePtr(NewMongoDBAtlasRoles(rolesModel)),
+		Labels:       conversion.NonEmptySliceToSlicePtr(NewMongoDBAtlasLabels(labelsModel)),
+		Scopes:       conversion.NonEmptySliceToSlicePtr(NewMongoDBAtlasScopes(scopesModel)),


why not have the New.. function return the pointer?

I'm trying to minimize the changes here, then there will a PR per resource to fix the update bugs, and check the readonly / createonly attributes, and improve the clarity as in your suggestion. Those changes will be more located (per resource)

as Agusting said before we'll probably get rid of all NonEmptySliceToSlicePtr in the next PRs

internal/service/onlinearchive/data_source_online_archive.go

andreaangiolillo · 2024-01-15T11:28:08Z

internal/service/onlinearchive/data_source_online_archive.go

@@ -216,12 +216,12 @@ func schemaOnlineArchive() map[string]*schema.Schema {
 }

 func dataSourceMongoDBAtlasOnlineArchiveRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
-	connV2 := meta.(*config.MongoDBClient).AtlasV2
+	connOldV2 := meta.(*config.MongoDBClient).OldAtlasV2


Could we use the name v20231001002AtlasSDK instead of OldAtlasV2? It is not easy to understand what we consider the old atlas v2. In addition, the new name will allow us to support multiple older versions if needed

I'm following the convention @oarbusi did in his previous PR, e.g. it's being used in federated_settings_identity_provider service

I missed that PR 😭 What is our opinion on the name used for the Atlas SDK?

maybe it's not the best name but I couldn't find a better name to suggest, the idea is that it's an SDK a little older than the current one used in most places.
also the idea is that this old SDK is only used until the corresponding service team fixes their API so use of this old sdk should be short duration

I think the correct naming would include the dates to avoid confusion if more parallel sdks are mantained

right now I think it's a premature optimization as we only have 2 sdks, y we intend to have the old one a short period of time. if more than 2 would be needed at some moment then I agree.

at the end I've added the date to the old sdk as per your recommendation

gssbzn · 2024-01-15T12:09:42Z

internal/common/conversion/type_conversion.go

@@ -56,3 +56,10 @@ func IsStringPresent(strPtr *string) bool {
 func MongoDBRegionToAWSRegion(region string) string {
 	return strings.ReplaceAll(strings.ToLower(region), "_", "-")
 }
+
+func NonEmptySliceToSlicePtr[T any](v []T) *[]T {


if slices only let's be more correct about it

Suggested change

func NonEmptySliceToSlicePtr[T any](v []T) *[]T {

func NonEmptySliceToSlicePtr[S ~[]E, E any]](v S) *S {

name changed to NonEmptyToPtr as it might called for slices or also arrays.

internal/service/alertconfiguration/data_source_alert_configuration.go

internal/service/atlasuser/data_source_atlas_users_test.go

gssbzn · 2024-01-15T12:13:44Z

internal/service/onlinearchive/data_source_online_archive.go

@@ -216,12 +216,12 @@ func schemaOnlineArchive() map[string]*schema.Schema {
 }

 func dataSourceMongoDBAtlasOnlineArchiveRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
-	connV2 := meta.(*config.MongoDBClient).AtlasV2
+	connOldV2 := meta.(*config.MongoDBClient).OldAtlasV2


I think the correct naming would include the dates to avoid confusion if more parallel sdks are mantained

AgustinBettati

LGTM besides pending comments

AgustinBettati · 2024-01-15T13:10:19Z

internal/common/conversion/type_conversion.go

@@ -56,3 +56,10 @@ func IsStringPresent(strPtr *string) bool {
 func MongoDBRegionToAWSRegion(region string) string {
 	return strings.ReplaceAll(strings.ToLower(region), "_", "-")
 }
+
+func NonEmptySliceToSlicePtr[T any](v []T) *[]T {


we will likely have a similar approach in CFN and CLI for this version upgrade, could be worth incorporating within SDK as a helper function and mention in upgrade guide.

@gssbzn what do you think?

on the other hand I think I'll be able to delete most of the uses of this function in next PRs

lantoli · 2024-01-15T15:22:29Z

internal/config/client.go

@@ -31,7 +31,7 @@ var (
 type MongoDBClient struct {
 	Atlas      *matlasClient.Client
 	AtlasV2    *atlasSDK.APIClient
-	OldAtlasV2 *oldAtlasSDK.APIClient // Needed to avoid sudden breaking changes in federated_settings_identity_provider resource. Will be removed in terraform-provider-1.16.0
+	OldAtlasV2 *oldAtlasSDK.APIClient // Needed to avoid sudden breaking changes in federated_settings_identity_provider and online_archive resources. Will be removed in terraform-provider-1.16.0


@andreaangiolillo @AgustinBettati @gssbzn I've added online_archive to the old sdk comment. but I would prefer to keep this PR changes as small as possible, I think old sdk renaming belongs to a different PR, it would affect client file and federated files apart from online_archive.

@andreaangiolillo @AgustinBettati @gssbzn hey, at the end I've added the date to old sdk in this PR in case you want to take a look. thanks

marcosuma · 2024-01-15T15:38:40Z

LGTM on my side, will wait once the comments are addressed.

lantoli force-pushed the CLOUDP-222424_sdk_update_pointers branch from eae0000 to 25ea035 Compare January 12, 2024 15:09

lantoli commented Jan 12, 2024

View reviewed changes

internal/common/conversion/type_conversion.go Outdated Show resolved Hide resolved

lantoli added 7 commits January 12, 2024 20:01

apply update-atlas-sdk

c28ae5e

revert changes to oldAdmin

32d630a

remove atlasSDK alias

2527ba6

use SlicePtrToSlice and NonEmptySliceToPtrSlice

2587fb3

TEMPORARY - remove DataProcessRegion to make it compile and run tests

404512e

fix linter: fieldalignment: struct with 32 pointer bytes could be 24 …

f93464f

…(govet)

fix project tests

5651c2b

lantoli force-pushed the CLOUDP-222424_sdk_update_pointers branch from 23793e5 to 5651c2b Compare January 12, 2024 19:02

lantoli added 2 commits January 12, 2024 20:02

Revert "TEMPORARY - remove DataProcessRegion to make it compile and r…

d0d2d67

…un tests" This reverts commit 404512e.

user old SDK for online archive as DataProcessRegion was removed

58abc5f

lantoli marked this pull request as ready for review January 12, 2024 20:14

lantoli requested a review from a team as a code owner January 12, 2024 20:14

marcosuma reviewed Jan 15, 2024

View reviewed changes

internal/common/conversion/type_conversion.go Outdated Show resolved Hide resolved

marcosuma reviewed Jan 15, 2024

View reviewed changes

internal/common/conversion/type_conversion.go Outdated Show resolved Hide resolved

lantoli added 3 commits January 15, 2024 08:19

update templates

06cc66e

rename to NonEmptySliceToSlicePtr

1d0f3fa

tests for slice functions

879a09b

lantoli commented Jan 15, 2024

View reviewed changes

internal/common/conversion/type_conversion_test.go Outdated Show resolved Hide resolved

gssbzn reviewed Jan 15, 2024

View reviewed changes

internal/common/conversion/type_conversion_test.go Outdated Show resolved Hide resolved

gssbzn reviewed Jan 15, 2024

View reviewed changes

internal/common/conversion/type_conversion_test.go Outdated Show resolved Hide resolved

gssbzn reviewed Jan 15, 2024

View reviewed changes

internal/common/conversion/type_conversion_test.go Outdated Show resolved Hide resolved

gssbzn reviewed Jan 15, 2024

View reviewed changes

internal/service/atlasuser/data_source_atlas_users_test.go Outdated Show resolved Hide resolved

gssbzn reviewed Jan 15, 2024

View reviewed changes

internal/service/alertconfiguration/data_source_alert_configuration.go Outdated Show resolved Hide resolved

gssbzn reviewed Jan 15, 2024

View reviewed changes

use Run in tests

0ab1d20

AgustinBettati reviewed Jan 15, 2024

View reviewed changes

internal/service/onlinearchive/data_source_online_archive.go Outdated Show resolved Hide resolved

remove SlicePtrToSlice

63671e7

andreaangiolillo reviewed Jan 15, 2024

View reviewed changes

lantoli requested review from gssbzn, AgustinBettati and marcosuma January 15, 2024 11:32

gssbzn reviewed Jan 15, 2024

View reviewed changes

AgustinBettati reviewed Jan 15, 2024

View reviewed changes

lantoli added 5 commits January 15, 2024 14:39

use GetResults

e9cb867

extract results

3e77d82

implement suggestion preallocating slice

46a67a1

rename to NonEmptyToPtr as it works for arrays and slices

ee26ed2

add online_archive to old sdk comment

5bdf6d0

lantoli commented Jan 15, 2024

View reviewed changes

comment NonEmptyToPtr

d475744

AgustinBettati approved these changes Jan 15, 2024

View reviewed changes

use date to refer to the old sdk version

94879f4

lantoli merged commit 02281f2 into master Jan 15, 2024
33 checks passed

lantoli deleted the CLOUDP-222424_sdk_update_pointers branch January 15, 2024 18:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Uses new Atlas Go SDK version #1831

chore: Uses new Atlas Go SDK version #1831

lantoli commented Jan 12, 2024 •

edited

Loading

gssbzn Jan 15, 2024

lantoli Jan 15, 2024 •

edited

Loading

lantoli Jan 15, 2024 •

edited

Loading

andreaangiolillo Jan 15, 2024 •

edited

Loading

lantoli Jan 15, 2024

andreaangiolillo Jan 15, 2024

lantoli Jan 15, 2024 •

edited

Loading

gssbzn Jan 15, 2024

lantoli Jan 15, 2024 •

edited

Loading

lantoli Jan 15, 2024

gssbzn Jan 15, 2024 •

edited

Loading

lantoli Jan 15, 2024

gssbzn Jan 15, 2024

AgustinBettati left a comment

AgustinBettati Jan 15, 2024

lantoli Jan 15, 2024

lantoli Jan 15, 2024

lantoli Jan 15, 2024

marcosuma commented Jan 15, 2024

	func NonEmptySliceToSlicePtr[T any](v []T) *[]T {
	func NonEmptySliceToSlicePtr[S ~[]E, E any]](v S) *S {

chore: Uses new Atlas Go SDK version #1831

chore: Uses new Atlas Go SDK version #1831

Conversation

lantoli commented Jan 12, 2024 • edited Loading

Description

Type of change:

Required Checklist:

Further comments

Choose a reason for hiding this comment

lantoli Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

lantoli Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

andreaangiolillo Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lantoli Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lantoli Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gssbzn Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AgustinBettati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcosuma commented Jan 15, 2024

lantoli commented Jan 12, 2024 •

edited

Loading

lantoli Jan 15, 2024 •

edited

Loading

lantoli Jan 15, 2024 •

edited

Loading

andreaangiolillo Jan 15, 2024 •

edited

Loading

lantoli Jan 15, 2024 •

edited

Loading

lantoli Jan 15, 2024 •

edited

Loading

gssbzn Jan 15, 2024 •

edited

Loading