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

Implemented /v1/catalog/service/:service with strong types #39

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Implemented /v1/catalog/service/:service with strong types #39

wants to merge 2 commits into from

Conversation

pierresouchay
Copy link
Owner

Will allow validating various datastructures (ex: serviceName with '/', Metadata limite keys/size...)

@stusmall
Copy link
Contributor

Unfortunately the keyword type doesn't introduce a new type, only a type alias. So while this will make code more readable it can lead to some surprises and won't introduce strong types. A good article on this can be found here: https://doc.rust-lang.org/1.8.0/book/type-aliases.html

I wrote a quick example of how you can have data easily move between these type aliases without the compiler stopping you: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=030d70ba1b7660bb76845ce43251d2ee

To get the strong typing we will need to implement each type as a new struct and provide the appropriate guidance to serde.

@pierresouchay
Copy link
Owner Author

@stusmall In the current implementation, I did implement the following real types:

  1. ConsulAddress(String) => used in several fields
  2. ConsulID(String) => used as an alias by ServiceID, NodeID: make sense because IDs are shared the same format, do you want me to create separate types?
  3. ConsulName(String) => used as an alias by ServiceName, NodeName: it is the same thing, ConsulName can be exposed as DNS labels, hence the need for validation at build time (see for instance the current PR in Consul: Node, Service, and Tag names only alphanumeric characters hashicorp/consul#7450 )

For now, 1, 2 and 3 seem Ok to me as it allows to enforce common semantics.

There are also those aliases:

  • ServiceTags = Vec : For those we could enforce values: it could in theory be enforced to disallow DNS invalid names
  • ServicePort = u16;
  • OptionalServicePort = Option;

I was wondering if moving slowly for no types to aliases and eventually real types do make sense or if we want directly to use strong types everywhere.

Can you tell me which types you don't want to be aliases?

@pierresouchay
Copy link
Owner Author

@stusmall Do you have comments regarding #39 (comment) ?

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.

2 participants