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

cleanup test namespace #42

Merged
merged 3 commits into from
Jul 30, 2024
Merged

cleanup test namespace #42

merged 3 commits into from
Jul 30, 2024

Conversation

apesternikov
Copy link
Contributor

kubectl failure in integration test setup script causes the script to exit prematurely so it_sidecar does not have a chance to perform the cleanup.
This PR adds a trap that would be executed on exit of the script whether it normal exit or by error. This also make the cleanup part of the it_sidecar obsolete.

Comment on lines 48 to +53
set +e

ns_cleanup() {
echo "Performing namespace ${NAMESPACE} cleanup..."
${KUBECTL} --kubeconfig=${KUBECONFIG} --cluster=${CLUSTER} --user=${USER} delete namespace --wait=false ${NAMESPACE}
}

Choose a reason for hiding this comment

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

The set +e command disables the immediate exit on error, which can lead to the script continuing execution even if critical commands fail. This can cause unexpected behavior and make debugging difficult. Consider handling errors explicitly where needed and re-enabling set -e after the critical section.

@@ -71,6 +72,8 @@ else
exit 1
fi
done

Choose a reason for hiding this comment

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

The while loop that generates a random namespace does not have a termination condition if the namespace creation keeps failing. This can lead to an infinite loop. Consider adding a maximum retry limit to avoid potential infinite loops.

Comment on lines 374 to 375
go func() {
err := stern.Run(ctx, *namespace, clientset, allowErrors, disablePodLogs)

Choose a reason for hiding this comment

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

The goroutine starting at line 374 calls stern.Run but does not handle any potential errors returned by this function. If stern.Run fails, the error will be lost, making debugging difficult.

Recommendation: Handle the error returned by stern.Run within the goroutine, possibly by logging it or sending it to a channel for further processing.

@fasterci fasterci deleted a comment from codereviewbot-ai bot Jul 30, 2024
@apesternikov apesternikov marked this pull request as ready for review July 30, 2024 21:05
@apesternikov apesternikov merged commit 39f94ee into main Jul 30, 2024
1 of 2 checks passed
@apesternikov apesternikov deleted the ap/cleanup-test-namespace branch August 30, 2024 19:18
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