-
Notifications
You must be signed in to change notification settings - Fork 178
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
chore: Enables mongodbatlas_push_based_log_export tests in CI and adds unit tests #2196
Conversation
tests are passing but showing "400 (request "CANNOT_ASSUME_ROLE") Atlas cannot assume the specified role" warning, don't know if it's important |
func TestNewTFPushBasedLogExport(t *testing.T) { | ||
currentTime := time.Now() | ||
|
||
testCases := []sdkToTFModelTestCase{ |
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.
knit: i think it's a bit better using a string map for the test name, e.g.: https://github.com/mongodb/terraform-provider-mongodbatlas/blob/master/internal/service/project/model_project_test.go#L550-L557
{ | ||
name: "Valid TF state", | ||
input: &pushbasedlogexport.TFPushBasedLogExportRSModel{ | ||
BucketName: types.StringValue(testBucketName), |
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.
to simplify test we might want to have in testBucketName the value of types.StringValue(testBucketName) so we can have just: BucketName: testBucketName,
same for all
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 don't see a lot of value in doing that, we're not only using types.StringValue(testBucketName) but also admin.PtrString() and the strings as is
assumeRoleFailedState = "ASSUME_ROLE_FAILED" | ||
unknown = "" | ||
sc500 = conversion.IntPtr(500) | ||
currentTime = time.Now() |
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 think it's ok here but sometimes using the current (real) time can lead to flaky tests
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.
let me know if you have any suggestions? I can update that if needed
@@ -45,3 +46,7 @@ func RandomEmail() string { | |||
func RandomLDAPName() string { | |||
return fmt.Sprintf("CN=%s-%[email protected],OU=users,DC=example,DC=com", prefixName, acctest.RandString(10)) | |||
} | |||
|
|||
func RandomS3BucketName() string { |
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.
nice!
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
expectedCreateReq: &admin.CreatePushBasedLogExportProjectRequest{ | ||
BucketName: testBucketName, | ||
IamRoleId: testIAMRoleID, | ||
PrefixPath: prefixPathEmpty, | ||
}, | ||
expectedUpdateReq: &admin.PushBasedLogExportProject{ | ||
BucketName: admin.PtrString(testBucketName), | ||
IamRoleId: admin.PtrString(testIAMRoleID), | ||
PrefixPath: admin.PtrString(prefixPathEmpty), | ||
}, |
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.
nice having both request models asserted under the same test
yes this is expected with the cloud provider access resource API. The resource then does retries to get a successful response. |
Description
Enables mongodbatlas_push_based_log_export tests in CI and adds unit tests
Link to any related issue(s): https://jira.mongodb.org/browse/CLOUDP-240975
Type of change:
Required Checklist:
Further comments