-
Notifications
You must be signed in to change notification settings - Fork 17
Fix multi-arch smoke setup and some flaky test #387
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
Conversation
MCK 1.3.0 Release NotesNew FeaturesMulti-Architecture SupportWe've added comprehensive multi-architecture support for the kubernetes operator. This enhancement enables deployment on IBM Power (ppc64le) and IBM Z (s390x) architectures alongside
Bug Fixes
Other Changes
|
@@ -40,7 +40,7 @@ def replica_set(namespace: str, custom_mdb_version) -> MongoDB: | |||
return resource.update() | |||
|
|||
|
|||
@fixture(scope="module") | |||
@fixture(scope="function") |
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.
we are editing the user and secret later - module scope means we wouldn't return the "updated" user/secret...
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.
blocking: Doesn't this also mean that we will revert any change made to the user?
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.
you are right, that whole setup is bad. Let me rework that whole structure. We should have function for reads and generally split up read and write...
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.
adressed here: 06effa5
ugh i remember that we had the idea of reworking all tests to follow a clear pattern such that we are not running into that again..
@@ -117,7 +117,6 @@ def test_user_can_authenticate_with_new_password(self): | |||
password="my-new-password", | |||
username="mms-user-1", | |||
auth_mechanism="SCRAM-SHA-256", | |||
attempts=20, |
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.
missed that from prior pr where we changed the default to 50...
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.
1m is way too close to the actual time required to pass the test. When it passed it needed 55 seconds
@@ -1475,7 +1475,7 @@ buildvariants: | |||
run_on: | |||
- rhel9-power-small | |||
- rhel9-power-large | |||
allowed_requesters: [ "patch", "github_tag" ] | |||
allowed_requesters: [ "patch", "github_tag" , "commit"] |
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.
rag bot says this is correct; also stated here: https://docs.devprod.prod.corp.mongodb.com/evergreen/Project-Configuration/Project-Configuration-Files#controlling-when-tasks-and-variants-run
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.
by this is correct - I mean merging to master will trigger this task
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.
Left some nits, looks good overall.
scripts/evergreen/setup_aws.sh
Outdated
if command -v aws &> /dev/null && aws --version &> /dev/null 2>&1; then | ||
echo "AWS CLI is already installed and working:" | ||
aws --version |
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.
We already ran the aws --version
command on line 52, so we don't really have to run it on 54 right? if we are just making sure that aws is working properly.
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.
addressed
scripts/evergreen/setup_aws.sh
Outdated
if command -v aws &> /dev/null && aws --version &> /dev/null 2>&1; then | ||
echo "AWS CLI v1 installed successfully:" | ||
aws --version |
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.
Same comment here.
https://github.com/mongodb/mongodb-kubernetes/pull/387/files#r2310251249
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.
addressed
@@ -1520,7 +1520,7 @@ buildvariants: | |||
tags: [ "e2e_test_suite", "e2e_smoke_release_test_suite" ] | |||
run_on: | |||
- ubuntu2204-arm64-large | |||
allowed_requesters: [ "patch", "github_tag" ] | |||
allowed_requesters: [ "patch", "github_tag", "commit"] |
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.
enabling these builds to be triggered on direct commits in addition to patches and GitHub tags
Can you please explain this a bit, just for my understanding. Do you mean, now this test will also be run when a commit is pushed to a PR branch? If yes, was not does if we just had patch?
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.
no - only direct commits to master - which means merges to mainline (mainline is master in our case) in evergreen terminology (more on the linked docs).
"Yes, setting allowed_requesters: ["commit"] is correct if you want your Evergreen task to run only on mainline (master) commits—i.e., after a branch is merged into master. This will prevent the task from running on PRs, patches, or other request types."
@@ -40,7 +40,7 @@ def replica_set(namespace: str, custom_mdb_version) -> MongoDB: | |||
return resource.update() | |||
|
|||
|
|||
@fixture(scope="module") | |||
@fixture(scope="function") |
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.
blocking: Doesn't this also mean that we will revert any change made to the user?
Summary
This pull request introduces several improvements to CI/CD configuration, test reliability, and scripting. The main changes are the expansion of allowed requesters for certain build variants, enhancements to the authentication test fixtures for better isolation and reliability, and a simplification of the AWS CLI installation script.
Test Improvements:
replica_set_scram_sha_256_connectivity.py
from module scope to function scope, ensuring a fresh environment for each test and reducing side effects between tests.try_load
and explicit secret management, improving test reliability and clarity.attempts
parameter from the authentication check in sharded cluster test, simplifying the test logic.buildvariants
sections of.evergreen.yml
, enabling these builds to be triggered on direct commits in addition to patches and GitHub tags. -> i hope that fixes smoke tests to run on master mergesinstall_aws_cli_pip()
inscripts/evergreen/setup_aws.sh
to check if AWS CLI is already installed and working before attempting installation, preventing unnecessary reinstalls.pip3
and improved error handling and messaging for installation failures.Proof
Checklist
skip-changelog
label if not needed