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.

Sign up for GitHub

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

Jump to bottom

Bootstrap gets error if the envd_buildkitd was stopped before #417

Merged
merged 3 commits into from
Jun 17, 2022
Merged

Bootstrap gets error if the envd_buildkitd was stopped before #417

merged 3 commits into from
Jun 17, 2022

Conversation

pingsutw
Copy link
Member

@pingsutw pingsutw commented Jun 17, 2022 •
edited
Loading

Signed-off-by: Kevin Su pingsutw@apache.org

Related to #397

image

pingsutw added 2 commits June 17, 2022 17:06
@pingsutw
Bootstrap gets error if the envd_buildkitd was stopped before
a1acac2 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
lint
1869485 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@gaocegege
Copy link
Member

Thanks for the contribution!
The tests are flakey. I will rerun it when I get a laptop.
The code itself LGTM!

@gaocegege
Copy link
Member

Or you can force push to trigger a new build.

@VoVAllen
Copy link
Member

Shall we try to restart the container here? @gaocegege

@gaocegege
Copy link
Member

I think it is better not to restart it, since we do not know if the container exits normally. Maybe we can print a warning to tell users they can remove or restart the container, WDYT @pingsutw @VoVAllen @Triple-Z

@Triple-Z
Copy link
Contributor

IMO, restart this container is acceptable. Is this container stateful?

@pingsutw
Add warning
88562b0 
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw
Copy link
Member Author

Yes, agreed, the container should restart by users themself.
Added warning. Thanks for the review.

Triple-Z
Triple-Z approved these changes Jun 17, 2022
View reviewed changes
Copy link
Contributor

@Triple-Z Triple-Z left a comment

Choose a reason for hiding this comment

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

LGTM

@gaocegege gaocegege merged commit 0c00005 into tensorchord:main Jun 17, 2022
@gaocegege
Copy link
Member

@all-contributors Please add @pingsutw for his code contribution

@allcontributors allcontributors bot mentioned this pull request Jun 17, 2022
Merged
@allcontributors
Copy link
Contributor

@gaocegege

I've put up a pull request to add @pingsutw! 🎉

@Triple-Z Triple-Z mentioned this pull request Jun 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Triple-Z Triple-Z Triple-Z approved these changes

Assignees
No one assigned
Labels
None yet
Projects
No open projects
envd sprint board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pingsutw @gaocegege @VoVAllen @Triple-Z