-
Notifications
You must be signed in to change notification settings - Fork 55
SREP-1164 : Update README file for running E2E Tests for CAD #517
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
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ratnam915 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #517 +/- ##
==========================================
+ Coverage 32.16% 32.68% +0.51%
==========================================
Files 37 37
Lines 2459 2472 +13
==========================================
+ Hits 791 808 +17
+ Misses 1609 1603 -6
- Partials 59 61 +2 🚀 New features to boost your workflow:
|
test/e2e/README.md
Outdated
## ONLY FOR LOCAL TESTING, THIS CONFIGURATION HAS TO BE REVERTED BACK BEFORE COMMIT AND PUSHING TO THE REPOSITORY | ||
|
||
Comment out #56,57 in configuration_anomaly_detection_tests.go and replace with the following code: | ||
|
||
ocme2eCli, err = ocme2e.New(ctx, ocmToken, clientID, clientSecret, ocmEnv) | ||
Expect(err).ShouldNot(HaveOccurred(), "Unable to setup E2E OCM Client") | ||
|
||
ocmCli, err = ocm.New(cadOcmFilePath) | ||
Expect(err).ShouldNot(HaveOccurred(), "Unable to setup ocm anomaly detection client") | ||
|
||
Add below statements in #50,#53 respectively | ||
|
||
ocmToken := os.Getenv("OCM_TOKEN") | ||
Expect(ocmToken).NotTo(BeEmpty(), "OCM_TOKEN must be set") | ||
|
||
## "!! PLEASE NOTE THAT SINCE OCM_TOKEN IS NOW DEPRECATED ABOVE LINES OF CODE HAVE TO BE REMOVED AND #56,57 HAVE TO BE UNCOMMENTED !!" |
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.
Can we ensure we don't need to modify the code to run this locally? That seems like a workaround ;) Ideally, running e2e tests with a target cluster should work programmatically. If you could provide a short script here, that would be very beneficial.
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.
Hi @typeid : The reason this workaround is being proposed from us is for OCM_CLIENT_ID and OCM_CLIENT_SECRET to work from a local machine a service account is required, if the service account is available we would not need to do this workaround and the test would work fine.
The script would not be required in that case, please let us know on how to proceed with this.
I can put a note in with a link to the SOP to create service account and enable it to be able to test the code in local.
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.
How about attempting to load the existing ocm token, and falling back to clientid and clientsecret?
That way it would work for both envs, correct?
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.
Also, how do we do this change after building the e2e?
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.
E.g.
import (
ocmConfig "github.com/openshift-online/ocm-common/pkg/ocm/config"
ocmConnBuilder "github.com/openshift-online/ocm-common/pkg/ocm/connection-builder"
)
...
cfg, err := ocmConfig.Load()
if err != nil {
clientID := os.Getenv("OCM_CLIENT_ID")
Expect(clusterID).NotTo(BeEmpty(), "OCM_CLIENT_ID must be set")
clientSecret := os.Getenv("OCM_CLIENT_SECRET")
Expect(clusterID).NotTo(BeEmpty(), "OCM_CLIENT_SECRET must be set")
ocme2eCli, err = ocme2e.New(ctx, "", clientID, clientSecret, ocmEnv)
Expect(err).ShouldNot(HaveOccurred(), "Unable to setup E2E OCM Client")
} else {
// Build connection based on local config
connection, err := ocmConnBuilder.NewConnection().Config(cfg).AsAgent("cad-local-e2e-tests").Build()
ocme2eCli = &ocme2e.Client{Connection: connection}
Expect(err).ShouldNot(HaveOccurred(), "Unable to setup E2E OCM Client")
}
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.
Thanks @typeid, this has significantly reduced overhead, the README file is fairly simple now.
I made the code changes and carried out local testing and it ran fine.
Also in other news i was comparing the Pagerduty alerts that we received https://redhat.pagerduty.com/service-directory/P4BLYHK/activity to the runtimes of the E2E pipeline -> https://prow.ci.openshift.org/job-history/gs/test-platform-results/logs/periodic-ci-openshift-osde2e-main-nightly-4.19-rosa-classic-sts
It looks like they are matching, however i don't see any logs from our test cases niether passing nor failing, should be consider closing the task for the environment variables syncing with E2E?
test/e2e/README.md
Outdated
7. Enable Debug Mode | ||
This enables detailed logging during test execution: | ||
export CAD_DEBUG=true |
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.
What is this variable for if we're not running the cadctl afterwards?
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.
This is again required for local testing and can be removed if we are following the service account mode
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.
CAD_DEBUG? That variable doesn't even exist anymore, and it should have had no influence on e2e :)
test/e2e/README.md
Outdated
5. AWS Credentials | ||
These are needed for interacting with the cluster. You can find them in the ~/.aws/credentials file. | ||
export AWS_ACCESS_KEY_ID=<your AWS access key ID> | ||
export AWS_SECRET_ACCESS_KEY=<your AWS secret access key> |
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.
Can we make this more generic to work on "target stage cluster"? The credentials in this file will not always match the cluster's AWS creds.
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.
Added another line to ensure cluster is created with a default profile and the credentials for the same profile are set in the environment variables in this step
@@ -9,6 +9,42 @@ To do this, following steps are recommended | |||
|
|||
ocm get /api/clusters_mgmt/v1/clusters/(cluster-id)/credentials | jq -r .kubeconfig > /(path-to)/kubeconfig |
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.
Not part of this PR, but the sentence above:
. Deploy your new version of operator in a test cluster
That doesn't seem to apply here.
"/label tide/merge-method-squash" |
@ratnam915: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
What type of PR is this?
(feature/bug/documentation/other)
What this PR does / Why we need it?
Special notes for your reviewer
Test Coverage
Guidelines for CAD investigations
Test coverage checks
Pre-checks (if applicable)