GitHub actions tests #293

coderGo93 · 2020-08-13T17:19:21Z

Description

Added a flag with env SKIP_TEST_EXTERNAL_CREDENTIALS where it's function is to skip acceptance tests that requires external credentials such as GCP,azure and aws

Added a github actions and has condition where it executes tests when the branch is master or is a pull request

Link to any related issue(s):

Type of change:

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the Terraform contribution guidelines
I have added tests that prove my fix is effective or that my feature works per HashiCorp requirments
I have added any necessary documentation (if appropriate)
I have run make fmt and formatted my code

Further comments

coderGo93 · 2020-08-14T00:36:18Z

Sorry for many commits, I tried many times since I cannot try locally without using third apps. Commits related to travis can be ignored

themantissa · 2020-08-17T15:59:40Z

@coderGo93 just want to check - the tests errors will be fixed in another PR per our slack, correct?

coderGo93 · 2020-08-17T16:45:39Z

@coderGo93 just want to check - the tests errors will be fixed in another PR per our slack, correct?

That's correct, @themantissa . I'll fix the errors that is not API related like unauthorized error or similar

themantissa

Good progress! A few questions/comments - let me know if you have questions. Thanks!

themantissa · 2020-08-17T17:59:19Z

.github/workflows/automated-test-acceptances.yml

+          MONGODB_ATLAS_PRIVATE_KEY: ${{ secrets.MONGODB_ATLAS_PRIVATE_KEY }}
+          MONGODB_ATLAS_PROJECT_ID: ${{ secrets.MONGODB_ATLAS_PROJECT_ID }}
+          MONGODB_ATLAS_ORG_ID: ${{ secrets.MONGODB_ATLAS_ORG_ID }}
+          DB_USERNAME: ${{ secrets.DB_USERNAME }}


This was not an env we created. It looked like, from the errors this was only for x509? Also, ideally any MongoDB Atlas specific variable keeps the MONGODB_ATLAS_ prefix.

Ok, the DB_USERNAME you meant, sure is for database user rule, you're right I'll add MONGODB_ATLAS_ at first

.github/workflows/automated-test-acceptances.yml

themantissa · 2020-08-17T22:50:24Z

.github/workflows/automated-test.yml

+      - name: Website
+        run: make tools && make website-lint
+      - name: Test
+        run: make test


Linter picking up missing newlines^

themantissa · 2020-08-17T22:53:53Z

mongodbatlas/data_source_mongodbatlas_custom_db_role_test.go

@@ -10,6 +10,7 @@ import (
 )

 func TestAccDataSourceMongoDBAtlasCustomDBRole_basic(t *testing.T) {
+	SkipTestExtCred(t)


This is weird - why does custom db role have an external credential? Looking at the test it is it because the pre check has checkPeeringEnvAWS? Why is that?

Totally agree, @coderGo93 you need to remove the checkPeeringEnvAWS because these tests don't need AWS Credentials, so these tests don't be skipped

Understood, I only added the func that requires additional credentials in precheck or similar

themantissa · 2020-08-17T22:54:43Z

mongodbatlas/data_source_mongodbatlas_custom_db_roles_test.go

@@ -10,6 +10,7 @@ import (
 )

 func TestAccDataSourceMongoDBAtlasCustomDBRoles_basic(t *testing.T) {
+	SkipTestExtCred(t)


Same comment as above, this shoulnd't depend on peering to test.

themantissa · 2020-08-17T23:02:09Z

mongodbatlas/resource_mongodbatlas_cluster_test.go

@@ -305,6 +305,7 @@ func TestAccResourceMongoDBAtlasCluster_basicAzure(t *testing.T) {
 }

 func TestAccResourceMongoDBAtlasCluster_basicGCP(t *testing.T) {
+	SkipTestExtCred(t)


Interesting - this is another one I'm surprised to see peering in the pre check. If it's a basic GCP cluster I don't see why we'd have peering involved - i.e. TestAccResourceMongoDBAtlasCluster_withGCPNetworkPeering should cover it.

themantissa

Just a couple questions and one nit on a missing EoF warning in the review view.

themantissa · 2020-08-19T21:42:15Z

.travis.yml

-script: make test
+    - name: "Test"
+      script:
+        - make test


This still shows no newline at the end of the file, it passed the linter but should probably fix it up.

weird, I added newline, gotta check again

I did not realize that is the travis, I'm not sure if at the end we should keep the .travis.yml if Github actions can do the same work and "faster"

We'll probably remove it once we get github actions working well but let's keep it for now.

themantissa · 2020-08-19T21:44:19Z

mongodbatlas/data_source_mongodbatlas_maintenance_window_test.go

-		Steps: []resource.TestStep{
-			{
+		Steps:     []resource.TestStep{
+			/*{


Why this so much in this test commented out?

because I skip it, if you want I can delete the code related with ASAP? and if I don't comment it will give an error about lint unused code

If we don't need it then removing it is fine - much cleaner than leaving unused code.

themantissa · 2020-08-19T21:49:20Z

mongodbatlas/resource_mongodbatlas_network_container_test.go

@@ -245,7 +245,7 @@ func testAccMongoDBAtlasNetworkContainerConfigAzure(projectID, cidrBlock, provid
 			project_id   		 = "%s"
 			atlas_cidr_block = "%s"
 			provider_name		 = "%s"
-			region			     = "US_EAST_2"
+			region			     = "US_EAST"


Why the change? (both are Azure regions https://docs.atlas.mongodb.com/reference/microsoft-azure/)

Because it fails with US_EAST_2 and I don't know how to delete it with mongodb ui, it doesn't appear that container the same can be said for GCP, that's why I changed it so it can pass, do you want me to change it back?

You can't delete containers in the MongoDB UI because we don't show it there, only via API. I'm not sure I follow on why it might be failing, what's the actual error? Might be showing us a real problem.

…hange to false in private ip mode to avoid error about deprecated and avoid lint errors

… instance scale and changed it back the region for container azure

…c for database user

…ort to rs primare attributes instead of decode state id

* chore: improved regex to allow to use a $ character * chore: added a new test case to verify the dabase name when it's imported

…lag for check external credentials for some testacc because it's not necessary

…fails in github actions but not locally

… a temporary, will solve later

…ions

PacoDw

If all the above comments were attended is LGMT 👍

themantissa

Also looking good - similar to the other PR just would like a call out in the PR description if any bugs in the actual resources were fixed so that the tests would pass. thanks!

mongodbatlas/provider_test.go

themantissa · 2020-08-26T23:00:11Z

mongodbatlas/resource_mongodbatlas_cloud_provider_snapshot_restore_job.go

@@ -331,7 +331,7 @@ func resourceMongoDBAtlasCloudProviderSnapshotRestoreJobImportState(d *schema.Re

 	d.SetId(encodeStateID(map[string]string{
 		"project_id":              *projectID,
-		"cluster_name":            *clusterName,
+		"cluster_name":            requestParameters.ClusterName,


I asked about similar fixes in the other pull requests. If we fix a bug while fixing the tests that should be noted in the PR, that way users know what was fixed. It's just a "nice to do" and makes understanding what happened easier.

This is an import that sometimes passes or fails, need to see why does happen, not sure if it's because there are many test acceptances running at same time. I changed this to verify it was the issue but it isn't, that's why I added a env var to skip test import if true in all test acceptance that imports because if you skip one another can appear with similar error.

Okay then if the change didn't fix anything and is not needed it shouldn't be included unless it's there for a reason since this isn't just the test but the actual resource code.

Ok, I'll change it back for now, thank you

mongodbatlas/resource_mongodbatlas_cluster.go

… is similar to multiple ip whitelist

themantissa

LGTM

coderGo93 requested review from PacoDw, shum, themantissa and marinsalinas August 14, 2020 00:34

coderGo93 changed the title ~~WIP: GitHub actions tests~~ GitHub actions tests Aug 14, 2020

coderGo93 removed the request for review from shum August 14, 2020 00:38

themantissa requested changes Aug 17, 2020

View reviewed changes

coderGo93 requested a review from themantissa August 19, 2020 05:04

coderGo93 force-pushed the github-actions-tests branch 2 times, most recently from 398bd53 to 76ba6c5 Compare August 19, 2020 18:16

themantissa requested changes Aug 19, 2020

View reviewed changes

themantissa mentioned this pull request Aug 19, 2020

Fixes many testacc #296

Merged

10 tasks

coderGo93 force-pushed the github-actions-tests branch 6 times, most recently from 9ea2c06 to 1b73007 Compare August 22, 2020 01:12

coderGo93 requested a review from themantissa August 26, 2020 15:54

Edgar López and others added 8 commits August 26, 2020 14:11

fix: fixes that show fail in testacc result

6b45132

test: commented in maintantance asap because is not needed for now, c…

b5c8ead

…hange to false in private ip mode to avoid error about deprecated and avoid lint errors

fix: error about delete cluster

5353797

test: deleted asap function for maintenance, fixes the bug of cluster…

5eac223

… instance scale and changed it back the region for container azure

test: fixes bug of destroy global cluster

0beb517

test: fix error about whitelist, added project resource inside testac…

9a648e4

…c for database user

test: changed database user to create project and get it, changed imp…

6f2de46

…ort to rs primare attributes instead of decode state id

Fix #285: Unable to import $external auth users (#297)

32754cd

* chore: improved regex to allow to use a $ character * chore: added a new test case to verify the dabase name when it's imported

Edgar López and others added 19 commits August 26, 2020 14:11

fix: added a newline at the end of file

23150b4

test: changed DB_USERNAME to MONGODB_ATLAS_DB_USERNAME, deleted the f…

1d15ce9

…lag for check external credentials for some testacc because it's not necessary

deleted the ref for checkout repo

55ace7b

docs: added environment variables for db username and teams ids

d35fcd0

test: added enviroment variable and validate to get teams ids

3207bfd

add env var for teams ids in yml tests

19f155a

reduced to 20 ip whitelist to see if it works now, somehow it always …

7c59b70

…fails in github actions but not locally

added ref to actual branch

8470f5b

test: fixed wrong data in config of database user test

47e6eda

test: changed import state

2e85b06

fix: fixed cloud_provider_snapshot_backup_policy

9636189

fix: fixes import for varios resources

705761f

test: added validation where you skip import for some resources, it's…

6e6ad17

… a temporary, will solve later

test: changed region aws to avoid error about capacity

531cb70

test: fixes some fails testacc

bd871e5

fix: added skip imports in some resources

8ef8ab3

fix: added skip imports in some resources

f1e1141

test: fixes some failed testacc

c0c0322

deleted the ref for checkout and travis so It can use only github act…

b60f105

…ions

PacoDw approved these changes Aug 26, 2020

View reviewed changes

coderGo93 force-pushed the github-actions-tests branch from 3f04316 to b60f105 Compare August 26, 2020 19:16

fix: changed to aws us east 1 to avoid error capacity for global cluster

648aafa

themantissa reviewed Aug 26, 2020

View reviewed changes

Edgar López added 3 commits August 26, 2020 18:46

changed it back as before

376184e

test: skip multiple custom db roles because sometimes fails, it seems…

d1a1d0e

… is similar to multiple ip whitelist

formated code to avoid linter error

be7bfba

coderGo93 requested a review from themantissa August 27, 2020 06:25

themantissa approved these changes Aug 27, 2020

View reviewed changes

coderGo93 merged commit c82481c into master Aug 27, 2020

coderGo93 deleted the github-actions-tests branch August 27, 2020 20:17

GitHub actions tests #293

GitHub actions tests #293

Uh oh!

Conversation

coderGo93 commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change:

Required Checklist:

Further comments

Uh oh!

coderGo93 commented Aug 14, 2020

Uh oh!

themantissa commented Aug 17, 2020

Uh oh!

coderGo93 commented Aug 17, 2020

Uh oh!

themantissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderGo93 Aug 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

themantissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderGo93 Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PacoDw left a comment

Choose a reason for hiding this comment

Uh oh!

themantissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderGo93 commented Aug 13, 2020 •

edited

Loading

coderGo93 Aug 18, 2020 •

edited

Loading

coderGo93 Aug 19, 2020 •

edited

Loading