-
Notifications
You must be signed in to change notification settings - Fork 193
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
OCM-7531 | test: automate id:OCP-61322,OCP-63164 create/delete tuning… #1951
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1951 +/- ##
==========================================
+ Coverage 22.91% 23.12% +0.21%
==========================================
Files 134 136 +2
Lines 20979 21967 +988
==========================================
+ Hits 4807 5080 +273
- Misses 15817 16511 +694
- Partials 355 376 +21 ☔ View full report in Codecov by Sentry. |
specPath, err := common.CreateTempFileWithContent(fmt.Sprintf(tuningConfigPayload_2, tuningConfigName_2, tuningConfigName_2)) | ||
defer os.Remove(specPath) | ||
Expect(err).ToNot(HaveOccurred()) | ||
_, err = tuningConfigService.EditTuningConfig(clusterID, tuningConfigName_2, "--spec-path", specPath) |
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.
It would be better to edit the config with a different config and then check if the value is updated.
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.
Yes I changed the spec content from tuning_config_payload_1 to tuning_config_payload_2 and then checked it later on
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 checked it in describe updated tuning_config as was said in the test case
AWSAccountID := whoamiData.AWSAccountID | ||
|
||
By("Create account-roles of hosted-cp in stable channel") | ||
output, err := ocmResourceService.CreateAccountRole("--mode", "auto", |
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 the case should create account role via "rosa create account-roles".
@yuwang-RH Would you help double confirm it?
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.
@yingzhanredhat But It is using rosa create account-roles command only, can you please check the logs once ? Thanks
It("create/delete hypershift account roles with managed policies - [id:61322]", | ||
labels.Critical, | ||
func() { | ||
defer func() { |
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.
Just curious as to why we use defer over using AfterEach
or AfterSuite
?
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.
Actually we use cluster.CleanResources() func in after suite, but in case of roles different test cases needs to delete different roles like account-roles, operator-roles, etc, so in this file we use actually defer func individually for specific cases
@@ -119,6 +120,18 @@ func (tcs *tuningConfigService) ListTuningConfigsAndReflect(clusterID string) (* | |||
return tcs.ReflectTuningConfigList(output) | |||
} | |||
|
|||
// Check the tuningConfig with the name exists in the tuningConfigList | |||
func (tuningConfigs TuningConfigList) IsExist(tcName string) (existed bool) { |
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 would call it IsPresent
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 changed it as you said, Thanks
… config and hypershift iam roles
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aaraj7, radtriste The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
@aaraj7: 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/test-infra repository. I understand the commands that are listed here. |
OCM-7531 | test: automate id:OCP-61322,OCP-63164 create/delete tuning config and hypershift iam roles
OCP-61322 output logs :- https://privatebin.corp.redhat.com/?d71ab35c58810608#BGCWvkmeqvY7v6UyZkq4kwWus9J2x8Q8ZLVQnbLkxXq6
OCP-63164 logs :- https://privatebin.corp.redhat.com/?512aa9cea36be45d#4bKqxKhqfvNaKymSu8vK5gZzgShcqNvvAQLtMQydBaUU