-
Notifications
You must be signed in to change notification settings - Fork 140
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
fix(KONFLUX-2311): Fix PVC function in load test #1064
fix(KONFLUX-2311): Fix PVC function in load test #1064
Conversation
/test ? |
@naftalysh: The following commands are available to trigger required jobs:
Use
In response to this:
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. |
/test load-test-ci-10u-10t |
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 have given comments on how to update the code to avoid duplication
cmd/loadTests.go
Outdated
@@ -1735,6 +1759,9 @@ func (h *ConcreteHandlerItsPipelines) validateItsPipeline(ctx *JourneyContext, a | |||
return false, nil | |||
} | |||
if IntegrationTestsPipelineRun.IsDone() { | |||
if !stage { | |||
h.handlePVCSforITS(threadIndex, framework, IntegrationTestsPipelineRun) |
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 function is a duplicate of handlePVCS function and it fails SonarCloud Duplicated Lines (%) on New Code
55.6% rule
The way to allow handlePVCSto be used for both handlers - ConcreteHandlerPipelines and ConcreteHandlerItsPipelines is -
- To change the below ConcreteHandlerItsPipelines struct definition
type ConcreteHandlerItsPipelines struct {
BaseHandler
}
To
type ConcreteHandlerItsPipelines struct {
*ConcreteHandlerPipelines // Embedding ConcreteHandlerPipelines
}
2 . Update the below
h.handlePVCSforITS(threadIndex, framework, IntegrationTestsPipelineRun)
To
h.handlePVCS(threadIndex, framework, IntegrationTestsPipelineRun)
- Delete function handlePVCSforITS
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.
Updated the code 👍
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 @siddardh-ra
I found there is a change needed, I create a review
/test load-test-ci-10u-10t |
load-test-ci-10u-10t seamed to fail the load test |
/hold |
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 this suggested change is needed to fix the struct embedding
The previous declaration caused an error message in the previous load-test-ci-10u-10t test
cmd/loadTests.go
Outdated
@@ -1666,7 +1673,7 @@ func (h *ConcreteHandlerPipelines) handlePVCS(threadIndex int, framework *framew | |||
} | |||
|
|||
type ConcreteHandlerItsPipelines struct { | |||
BaseHandler | |||
*ConcreteHandlerPipelines // Embedding ConcreteHandlerPipelines |
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 should change it to the below to fix the struct embedding
ConcreteHandlerPipelines // Embedding ConcreteHandlerPipelines
/test load-test-ci-10u-10t |
Hi @siddardh-ra Now check it in GITHUB Action load tests workflow while pointing to your branch |
@siddardh-ra: The following tests failed, say
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. |
/test load-test-ci-10u-10t-nodejs |
/test ? |
@naftalysh: The following commands are available to trigger required jobs:
Use
In response to this:
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. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: naftalysh 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 |
/unhold |
/lgtm |
/test load-test-ci-10u-10t |
Update PVC function for ITS Add handling also for ITS pipelineRuns Signed-Off-By: Siddardh R A <[email protected]>
1b8d021
to
f4bc89d
Compare
Quality Gate passedIssues Measures |
/test load-test-ci-10u-10t |
/lgtm |
Update PVC function for ITS
Add handling also for ITS pipelineRuns
Signed-Off-By: Siddardh R A [email protected]
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
Issue ticket number and link
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: