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

feat(rust): improvements to portals commands arguments #8618

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

feat(rust): improvements to portals commands arguments #8618

wants to merge 1 commit into from

Conversation

adrianbenavides
Copy link
Member

@adrianbenavides adrianbenavides commented Nov 11, 2024 •
edited
Loading

  • Inlet create commands now have a positional argument to define the portal's name. This argument replaces the current alias or addr. Old arguments are kept as deprecated flags. To ensure backward compatibility, if a deprecated flag is used, it will take precedence over the positional argument
  • Outlet create commands also have the positional argument, as an alternative to the --from argument. In this case, we'll keep --from as a valid alternative (not deprecated).
  • InfluxDB commands now have their own args definition. This fixes the issue that was showing the TCP commands docs in the InfluxDB commands docs
  • The arguments using HostnamePort now use SchemeHostnamePort, allowing the user to optionally specify the scheme (tcp, tls or udp). The --tls and --udp flags have been deprecated (but kept), as this information can now be derived from the address arguments. To ensure backward compatibility, if a deprecated flag is used, it will take precedence over an address with an explicit scheme
  • Other args renames (with old names as valid aliases)
  • Unhide --allow flag in all the commands

@adrianbenavides adrianbenavides force-pushed the adrian/args-updates-portal-commands branch 2 times, most recently from 3b584d0 to 21a6fbe Compare November 12, 2024 08:48
@adrianbenavides adrianbenavides marked this pull request as ready for review November 12, 2024 09:08
@adrianbenavides adrianbenavides requested a review from a team as a code owner November 12, 2024 09:08
@adrianbenavides adrianbenavides marked this pull request as draft November 12, 2024 09:10
@adrianbenavides adrianbenavides force-pushed the adrian/args-updates-portal-commands branch 12 times, most recently from 14f951b to 91bb33a Compare November 13, 2024 08:45
@adrianbenavides adrianbenavides marked this pull request as ready for review November 13, 2024 08:46
@adrianbenavides adrianbenavides force-pushed the adrian/args-updates-portal-commands branch from 91bb33a to a5f0e8c Compare November 18, 2024 14:14
@adrianbenavides adrianbenavides mentioned this pull request Dec 10, 2024
Merged
etorreborre
etorreborre previously approved these changes Dec 10, 2024
View reviewed changes
Copy link
Member

@etorreborre etorreborre left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry if my comments are a bit out of order. This is a good step forward but I think that could be even clearer in the naming of "addresses" so that there no ambiguity in the description on what an "address" is. Feel free to open another PR for modification.

implementations/rust/ockam/ockam_api/src/nodes/registry.rs
@@ -346,6 +346,10 @@ mod tests {
}

fn outlet_info(worker_addr: Address) -> OutletInfo {
OutletInfo::new(HostnamePort::new("127.0.0.1", 0), Some(&worker_addr), true)
OutletInfo::new(
HostnamePort::new("127.0.0.1", 0).unwrap(),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could make a function HostnamePort::localhost(u16) that is guaranteed to succeed, instead of unwrapping here.

implementations/rust/ockam/ockam_command/src/influxdb/inlet/static/long_about.txt Outdated Show resolved Hide resolved
implementations/rust/ockam/ockam_command/src/influxdb/outlet/create.rs Outdated Show resolved Hide resolved
implementations/rust/ockam/ockam_command/src/influxdb/outlet/create.rs
)]
pub allow: Option<PolicyExpression>,

/// Use eBPF and RawSocket to access TCP packets instead of TCP data stream.
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be nice to either point to some documentation, or to explain that the overall goal is to support an end to end TCP connection, even across several Ockam nodes.

implementations/rust/ockam/ockam_command/src/kafka/consumer/create.rs Outdated Show resolved Hide resolved
implementations/rust/ockam/ockam_command/src/kafka/inlet/create.rs Outdated Show resolved Hide resolved
implementations/rust/ockam/ockam_command/src/influxdb/inlet/create.rs Outdated
#[arg(long, display_order = 900, id = "NODE_NAME", value_parser = extract_address_value)]
pub at: Option<String>,

/// Address on which to accept InfluxDB connections, in the format <scheme>://<address>:<port>.
Copy link
Member

Choose a reason for hiding this comment

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

I think that we should keep the name Address for the full <scheme>://<hostname>:<port> format and then refer to each component separately with those names. For example "the default hostname is 127.0.0.1".

Or maybe even say base URL instead of Address? (it's not really a URL because there's no resource path). This would leave the name Address for our multi-addresses like /project/default.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated args docs to use the same words consistently, not sure if that's all you meant here.

implementations/rust/ockam/ockam_command/src/kafka/outlet/create.rs
@@ -27,26 +27,35 @@ use crate::{
/// Create a new Kafka Outlet
#[derive(Clone, Debug, Args)]
pub struct CreateCommand {
/// Address of your Kafka Outlet, which is part of a route used in other commands.
/// This unique address identifies the Kafka Outlet worker on the Node on your local machine.
/// Examples are `/service/my-outlet` or `my-outlet`.
Copy link
Member

Choose a reason for hiding this comment

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

Can those 2 be really used interchangeably? If ${name} is going to be injected somewhere, it will have to be of one form or the other I guess? Unless we always prepend /service when it doesn't exist? Maybe we should have a dedicated type instead of String for those things.

Copy link
Member Author

Choose a reason for hiding this comment

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

This command is missing a value parser actually. It should use the extract_address_value to parse the multiaddr into a valid Address.

@adrianbenavides adrianbenavides force-pushed the adrian/args-updates-portal-commands branch from a5f0e8c to 7e7aa90 Compare December 18, 2024 11:49
@adrianbenavides adrianbenavides force-pushed the adrian/args-updates-portal-commands branch from 7e7aa90 to 853cc48 Compare December 18, 2024 12:32
@adrianbenavides
Copy link
Member Author

I've created issues for the open comments: #8710, #8711, #8712

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etorreborre etorreborre Awaiting requested review from etorreborre

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@adrianbenavides @etorreborre