Skip to content
This repository has been archived by the owner on Oct 31, 2023. It is now read-only.

Cluster seed validation for CLI args (and interactive) #202

Merged
merged 9 commits into from
Nov 30, 2021

Conversation

StuartHarris
Copy link
Contributor

@StuartHarris StuartHarris commented Nov 19, 2021

This PR implements some more of #192 by introducing ClusterSeed for the wash call command args and interactive input.

Note there is a remaining question about what a default ClusterSeed should be (if anything), as this was previously an empty string (which won't parse now).

  • introduce ClusterSeed
  • write some unit tests for Id and Seed parsing
  • revisit ClusterSeed::default() (wraps String::default())

Also added a .rustfmt.toml to allowing merging imports, but happy to revert this if you think the imports are now "too merged" :-)

@StuartHarris StuartHarris force-pushed the cluster-seed-arg branch 4 times, most recently from 57560e6 to 5032140 Compare November 19, 2021 14:54
Signed-off-by: Stuart Harris <stuart.harris@red-badger.com>
Signed-off-by: Stuart Harris <stuart.harris@red-badger.com>
Signed-off-by: Stuart Harris <stuart.harris@red-badger.com>
Signed-off-by: Stuart Harris <stuart.harris@red-badger.com>
Signed-off-by: Stuart Harris <stuart.harris@red-badger.com>
@StuartHarris StuartHarris changed the title introduce ClusterSeed Cluster seed validation for CLI args (and interactive) Nov 19, 2021
@StuartHarris StuartHarris marked this pull request as ready for review November 22, 2021 11:31
@StuartHarris
Copy link
Contributor Author

@brooksmtownsend opening this up for review, but would love to get your opinion on what ClusterSeed::default() should be. Before, it was an empty string (although not in a happy code path), so it may not matter what it is in reality (but I had to make it something that would now parse, so I made is SC + 54 x 0 :-)

autodidaddict
autodidaddict previously approved these changes Nov 22, 2021
Copy link
Member

@autodidaddict autodidaddict left a comment

Choose a reason for hiding this comment

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

LGTM, though I'd like @brooksmtownsend to double-check this to make sure it works fine in conjunction with the automatic configuration reading ~/.wash/host_config.json

Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

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

As far as I can tell this looks good with wash ctx. Turns out seed keys are 58 characters so valid keys won't parse with this iteration. After doing some testing I found a few more requests.

src/id.rs Outdated Show resolved Hide resolved
src/id.rs Show resolved Hide resolved
src/call.rs Show resolved Hide resolved
Signed-off-by: Stuart Harris <stuart.harris@red-badger.com>
Signed-off-by: Stuart Harris <stuart.harris@red-badger.com>
Signed-off-by: Stuart Harris <stuart.harris@red-badger.com>
Signed-off-by: Stuart Harris <stuart.harris@red-badger.com>
@brooksmtownsend brooksmtownsend merged commit cf61264 into wasmCloud:main Nov 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants