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

feat (kuberenetesDeploy) allow adding a timeout to the helm test commands #4310

Merged
merged 8 commits into from
Apr 24, 2023

Conversation

anilkeshav27
Copy link
Member

Changes

helm test command can now be made to wait with the param helmTestWaitSeconds . this gives a little more flexibility to allow long running test to complete before timing out

  • Tests
  • Documentation

@anilkeshav27 anilkeshav27 requested a review from a team as a code owner March 30, 2023 08:57
cmd/kubernetesDeploy.go Outdated Show resolved Hide resolved
resources/metadata/kubernetesDeploy.yaml Outdated Show resolved Hide resolved
@vstarostin
Copy link
Member

After taking a look at the helm documentation I think this parameter is not necessary for the kubernetesDeploy step (at least for the current implementation of the step), since the step uses the —wait parameter for the helm upgrade command, which means that helm will not return until deploy is in a ready state --> the helm test command will not be started until previous one is done.

From helm docu regarding the wait param: will wait until all Pods, PVCs, Services, and minimum number of Pods of a Deployment, StatefulSet, or ReplicaSet are in a ready state before marking the release as successful. It will wait for as long as --timeout.
https://helm.sh/docs/helm/helm_upgrade/#:~:text=version%20is%20used-,%2D%2Dwait,-if%20set%2C%20will

I think the wait parameter for the helm test command is mainly used for waiting until deployment is in a ready state, after that tests can be triggered.
Since in our case the helm upgrade takes care of it, the wait param for the helm test is not necessary.

Do we have another reason to wait for the execution of the helm test besides waiting until deployment is in a ready state?

@vstarostin vstarostin requested a review from a team April 2, 2023 16:13
@anilkeshav27
Copy link
Member Author

After taking a look at the helm documentation I think this parameter is not necessary for the kubernetesDeploy step (at least for the current implementation of the step), since the step uses the —wait parameter for the helm upgrade command, which means that helm will not return until deploy is in a ready state --> the helm test command will not be started until previous one is done.

From helm docu regarding the wait param: will wait until all Pods, PVCs, Services, and minimum number of Pods of a Deployment, StatefulSet, or ReplicaSet are in a ready state before marking the release as successful. It will wait for as long as --timeout. https://helm.sh/docs/helm/helm_upgrade/#:~:text=version%20is%20used-,%2D%2Dwait,-if%20set%2C%20will

I think the wait parameter for the helm test command is mainly used for waiting until deployment is in a ready state, after that tests can be triggered. Since in our case the helm upgrade takes care of it, the wait param for the helm test is not necessary.

Do we have another reason to wait for the execution of the helm test besides waiting until deployment is in a ready state?

Do we have another reason to wait for the execution of the helm test besides waiting until deployment is in a ready state?

its about when the test themselves run more than 300 seconds and not for the deployment to be in the ready state. Even when the deployment is in ready state , the test themselves can run more than 300s

@vstarostin
Copy link
Member

its about when the test themselves run more than 300 seconds and not for the deployment to be in the ready state. Even when the deployment is in ready state , the test themselves can run more than 300s

ah...yes, sorry, I missed it.

@vstarostin vstarostin requested review from a team and removed request for a team April 20, 2023 12:38
Copy link
Member Author

@anilkeshav27 anilkeshav27 left a comment

Choose a reason for hiding this comment

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

@vstarostin , your changes looks good, could you please approve it, since i cannot approve my own changes

@vstarostin
Copy link
Member

/it-go

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@vstarostin vstarostin merged commit 17c9f5c into master Apr 24, 2023
@vstarostin vstarostin deleted the anil/timeoutHelmTest branch April 24, 2023 12:58
maxatsap pushed a commit to maxatsap/jenkins-library that referenced this pull request Jul 23, 2024
…ands (SAP#4310)

* adding a timeout for helm test

* extending test cases

* Upadate the helmTestWaitSeconds parameter

* Add timeout parameter for helm test command

* Update tests

---------

Co-authored-by: Vyacheslav Starostin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants