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

cmd/network: Extract absolute path of local endpoint in add-local #350

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

abukosek
Copy link
Contributor

Closes #273.

Copy link

netlify bot commented Jan 21, 2025

Deploy Preview for oasisprotocol-cli canceled.

Name Link
🔨 Latest commit 9d951d5
🔍 Latest deploy log https://app.netlify.com/sites/oasisprotocol-cli/deploys/67911dc27a64bd000855fa64

@abukosek abukosek force-pushed the andrej/bugfix/add-local-abspath branch from 598c7aa to 5c33d25 Compare January 21, 2025 22:15
@abukosek abukosek marked this pull request as ready for review January 21, 2025 22:18
@abukosek abukosek requested a review from matevz January 21, 2025 22:18
cmd/network/add_local.go Outdated Show resolved Hide resolved
cobra.CheckErr(fmt.Errorf("malformed path in RPC endpoint: %w", err))
}
net.RPC = parsedRPC.String()

Copy link
Member

Choose a reason for hiding this comment

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

While you're at it, can you also check whether parsedRPC.Opaque() matches local file and print something like Warning: File ${parsedRPC.Opaque()} exists locally. Did you mean unix:${parsedRPC.Opaque()}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, but parsedRPC.Opaque comes from the original RPC endpoint, which has to start with unix: anyway or the validation will fail. I'm not sure what this check is supposed to accomplish.

Copy link
Member

Choose a reason for hiding this comment

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

This bad UX existed before:

$ oasis net add test .oasis-node3/data_validator/internal.sock 
Error: rpc error: code = Unavailable desc = name resolver error: produced zero addresses

Copy link
Contributor Author

@abukosek abukosek Jan 22, 2025

Choose a reason for hiding this comment

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

OK, but this is a totally different issue :) The original issue never mentioned adding this functionality to the add command, just to add-local.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the extra functionality, please retry :)

Copy link
Member

Choose a reason for hiding this comment

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

Can you also add the same behavior to add-local:

$ ./oasis net add-local test internal.sock 
Error: rpc-endpoint 'internal.sock' is not local

@abukosek abukosek force-pushed the andrej/bugfix/add-local-abspath branch 2 times, most recently from afb1700 to 7e2165a Compare January 22, 2025 13:55
Copy link
Member

@matevz matevz left a comment

Choose a reason for hiding this comment

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

Please append unix: for add-local as well. Otherwise looks good. Thanks!

@abukosek abukosek force-pushed the andrej/bugfix/add-local-abspath branch from 7e2165a to 9d951d5 Compare January 22, 2025 16:33
@abukosek abukosek merged commit 2a03a8f into master Jan 22, 2025
4 checks passed
@abukosek abukosek deleted the andrej/bugfix/add-local-abspath branch January 22, 2025 16:43
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.

network add-local: Extract absolute path for internal.sock
2 participants