Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
parallel e2e testing with different kubeconfigs #1020
base: master
Are you sure you want to change the base?
parallel e2e testing with different kubeconfigs #1020
Changes from 15 commits
3800d03
1561b6a
d7947cc
d9efd7f
ccc9a6d
f8a786d
56c0634
527e501
0f0e07e
4826fa1
0764631
86586a4
2df49bb
cd250f5
6958dce
347d28d
2b85e9f
b678b17
400c0ff
4c0b935
4031097
edab06c
6f55555
98b4a0d
8b72d6c
5154223
bc10e92
c1fd100
eb7c5a1
a2952f6
95d6371
50d3a81
129e7fe
7029681
7bc2a2d
4c7b111
842273f
ba66727
a13805f
a947c8c
18442f6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Question: Is it an actual timeout issue or can Zookeeper not start for some reason and we retry until we reach a timeout?
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.
Here you can see the problem: https://github.com/banzaicloud/koperator/runs/15416787902
It would be nice to get more information why is this fail (like pod status)
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.
Because in local environment all of the tests passed when multiple k8s cluster is used in parallel way I suspect that the problem is with timeouts or something else on github
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.
Please test the PR with multicluster input on your local.
This PR is not about to run tests on github on multiple kind cluster.
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.
Same as on line 56, I don't think we need this comment.
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 you please explain why should we remove this?
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'm really not sure what sense it makes to have the explanation for this timeout here. For giving information to reviewers it would be fine as a review comment but I don't think this needs to be a comment on 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.
+1 that the comment
"// Increased from 4 to 7 for multiple kind"
can be removed since it only provides the context about why the change is made, but it doesn't provide any valuable info itself along with the source code.Or perhaps we can change the comment to something like this:
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 can remove but in this case who will face with this issue on multiple kind cluster on github will not know what was the original setting for one kind cluster setup.
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 default values and the reason of their changes are valuable in this case.
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.
Because the increased timeout was not helped, I put back to the original value and I removed the comment
fixed: 129e7fe