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: add uds ui cmd #917

Merged
merged 20 commits into from
Sep 17, 2024
Merged

feat: add uds ui cmd #917

merged 20 commits into from
Sep 17, 2024

Conversation

decleaver
Copy link
Collaborator

Description

Adds uds ui which launches uds runtime

Related Issue

Relates to #870

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

  • Test, docs, adr added or updated as needed

@decleaver decleaver changed the title feat: add uds ui cmd feat: add uds ui cmd Sep 9, 2024
@decleaver decleaver marked this pull request as ready for review September 9, 2024 20:55
src/cmd/uds.go Outdated Show resolved Hide resolved
@UncleGedd
Copy link
Collaborator

UncleGedd commented Sep 10, 2024

Can we get around having to add the binaries to the repo? Also we shouldn't need to bundle all versions of Runtime into the binary. For example the darwin-arm64 CLI should embed only the darwin-arm64 Runtime. Thoughts?

@decleaver
Copy link
Collaborator Author

Can we get around having to add the binaries to the repo? Also we shouldn't need to bundle all versions of Runtime into the binary. For example the darwin-arm64 CLI should embed only the darwin-arm64 Runtime. Thoughts?

Good point, I can update it to only bundle the matching runtime binary. As far as adding the binaries to the repo, have some thoughts... pros/cons to different approaches.

@decleaver decleaver marked this pull request as draft September 10, 2024 16:01
@decleaver decleaver marked this pull request as ready for review September 10, 2024 20:39
src/cmd/uds.go Outdated Show resolved Hide resolved
Co-authored-by: UncleGedd <42304551+UncleGedd@users.noreply.github.com>
@UncleGedd
Copy link
Collaborator

Great work! I explored vendoring for a bit before realizing that was a dead end. One con to the embedding binary approach is that we're creating tmp dirs with the runtime binary on the user's system, and those tmp dirs can potentially be abandoned if the process is killed and the defer function doesn't get called (like when the user runs ctrl-c)

This can be a separate issue, but I'd like to see the following scenarios handled:

  1. What happens when the user hits ctrl-c? Can we force a cleanup of the tmp dir?
  2. Besides ctrl-c is there a graceful way to exit uds ui?

@decleaver
Copy link
Collaborator Author

  • What happens when the user hits ctrl-c? Can we force a cleanup of the tmp dir?
  • Besides ctrl-c is there a graceful way to exit uds ui?

Created follow on issue: #922

src/cmd/ui.go Outdated Show resolved Hide resolved
@UncleGedd UncleGedd merged commit 4aac812 into main Sep 17, 2024
16 checks passed
@UncleGedd UncleGedd deleted the uds-ui branch September 17, 2024 19:44
This pull request was closed.
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