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

Revert "Print out logs of failed container" #189

Closed
wants to merge 1 commit into from

Conversation

sylwiaszunejko
Copy link
Collaborator

@sylwiaszunejko sylwiaszunejko commented Jun 4, 2024

Reverts #188

This change not only prints out the logs if container is unhealthy but also exits and manage to not run the tests when there is nothing wrong with the containers.

@mykaul
Copy link

mykaul commented Jun 4, 2024

Please add a reason for reverting a commit in the revert commit message.

@sylwiaszunejko
Copy link
Collaborator Author

Please add a reason for reverting a commit in the revert commit message.

Done, sorry for merging that without being 100% sure that it is right and creating this whole situation

@mykaul
Copy link

mykaul commented Jun 4, 2024

Please add a reason for reverting a commit in the revert commit message.

Done, sorry for merging that without being 100% sure that it is right and creating this whole situation

No worries - this is not the issue at all. The issue is that it'd be great to know why we reverted a commit.

This change not only prints out the logs if container is
unhealthy but also exits and manage to not run the tests when
there is nothing wrong with the containers.
@sylwiaszunejko sylwiaszunejko force-pushed the revert-188-fix-integration-sh branch from d725620 to 95fb704 Compare June 4, 2024 07:05
@sylwiaszunejko
Copy link
Collaborator Author

I added more detailed commit message

@dkropachev
Copy link
Collaborator

@sylwiaszunejko , @mykaul, i should have kept that PR as draft, sorry.

Let's better merge this one, it will fix the issue but keep printing logs on error

@sylwiaszunejko
Copy link
Collaborator Author

@dkropachev I think it would be better to first merge this, then merge #187 and when the CI will be fixed we could add extra logging, because now CI on #190 won't pass

@dkropachev
Copy link
Collaborator

@dkropachev I think it would be better to first merge this, then merge #187 and when the CI will be fixed we could add extra logging, because now CI on #190 won't pass

But it won't pass because test is running on an image that don't support tablets:

 error: the argument ('tablets') for option '--experimental-features' is invalid

Copy link

@avelanarius avelanarius left a comment

Choose a reason for hiding this comment

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

LGTM, but maybe we should merge #190 instead

@mykaul
Copy link

mykaul commented Jun 4, 2024

@dkropachev I think it would be better to first merge this, then merge #187 and when the CI will be fixed we could add extra logging, because now CI on #190 won't pass

But it won't pass because test is running on an image that don't support tablets:

 error: the argument ('tablets') for option '--experimental-features' is invalid

And it's not going to be needed when 6.0 comes out (soon! soon!)

@sylwiaszunejko sylwiaszunejko deleted the revert-188-fix-integration-sh branch November 27, 2024 15:35
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.

4 participants