Skip to content
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

GODRIVER-2237: Run KMS KMIP spec and prose tests in Evergreen #816

Merged
merged 9 commits into from Nov 23, 2021

Conversation

gabbyasuncion
Copy link
Contributor

GODRIVER-2237

Configures an Evergreen build variant to test the KMS KMIP spec and prose tests. Passes according to this patch.

Copy link
Contributor

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Evergreen is complaining about your config.yml file being invalid; it's because of alignment issues. See my "suggested change" comments on how to fix that. Can you also reconfigure the associated patch to run KMS KMIP Ubuntu 18.04 so we can see that it succeeds?

.evergreen/config.yml Outdated Show resolved Hide resolved
.evergreen/config.yml Show resolved Hide resolved
.evergreen/config.yml Outdated Show resolved Hide resolved
.evergreen/config.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! 🧑‍🔧

Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a typo fix.

.evergreen/config.yml Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@benjirewis
Copy link
Contributor

As discussed offline, let's first merge PR #812 once you're done addressing comments there, rebase this PR on master, and make sure that the new/updated client-side-encryption tests are skipped on every task besides test-kms-kmip (since the correct servers will not be running) and pass on test-kms-kmip (where the correct servers are running).

@kevinAlbs
Copy link
Contributor

As discussed offline, let's first merge PR #812 once you're done addressing comments there, rebase this PR on master, and make sure that the new/updated client-side-encryption tests are skipped on every task besides test-kms-kmip (since the correct servers will not be running) and pass on test-kms-kmip (where the correct servers are running).

SGTM. A possible way to do this is to set an environment variable in the test-kms-kmip task. And skip the tests that are run as part of the test-kms-kmip task if that environment variable is not set:

Example with the TestClientSideEncryptionSpec/kmipKMS test. Update client_side_encryption_spec.go:

        for _, fileName := range jsonFilesInDir(t, path.Join(dataPath, encryptionSpecName)) {
                t.Run(fileName, func(t *testing.T) {
+                       if fileName == "kmipKMS.json" && "" == os.Getenv("KMS_MOCK_SERVERS_RUNNING") {
+                               t.Skipf("Skipping test as KMS_MOCK_SERVERS_RUNNING is not set")
+                       }
                        runSpecTestFile(t, encryptionSpecName, fileName)
                })
        }

Copy link
Contributor

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM even more!

Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@gabbyasuncion gabbyasuncion merged commit 517aca9 into mongodb:master Nov 23, 2021
@gabbyasuncion gabbyasuncion deleted the GODRIVER-2237 branch November 23, 2021 16:58
faem pushed a commit to kubedb/mongo-go-driver that referenced this pull request Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants