Add modeled types for URLs and Kubernetes resources #549
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zmrow
reviewed
Nov 21, 2019
| @@ -282,3 +303,401 @@ mod test_valid_identifier { | |||
| assert!(Identifier::try_from("💝").is_err()); | |||
| } | |||
| } | |||
|
|
|||
| // =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= | |||
Comment on lines
+327
to
+330
| // It's very common to specify URLs without a scheme, so we add one and see if that | ||
| // fixes parsing. | ||
| let prefixed = format!("http://{}", input); | ||
| if let Ok(_) = prefixed.parse::<url::Url>() { |
This improves errors when API requests are formatted or specified incorrectly, for example mixing up fields in user data.
|
This push just clarifies error messages for unit tests that fail, making the Url tests consistent with others. |
etungsten
reviewed
Nov 21, 2019
|
|
||
| #[snafu(display("{} must match '{}', given: {}", thing, pattern, input))] | ||
| Pattern { | ||
| thing: String, |
There was a problem hiding this comment.
small nit: Could there be a better way to represent this field? Maybe modeled-type?
There was a problem hiding this comment.
It's not exactly that, it's a descriptive string regarding the... thing... the error might be about. It's a bit more general than modeled type. Perhaps desc?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This improves errors when API requests are formatted or specified incorrectly,
for example mixing up fields in user data.
Depends on #546.
Testing done:
All of the updated fields now give clear(er) errors if given invalid values.
invalid cluster name:
invalid label key:
invalid label value:
invalid taint value:
invalid target base url:
invalid metadata base url:
invalid ntp server:
invalid host container source:
invalid api server:
Here I screwed up a boolean:
You'll note that some of the errors look the same and don't specify the exact setting that was invalid. I don't think people are likely to specify the same invalid value multiple times and be confused, but I recognize that it could be improved and would like to in the future. Same point for potentially over-verbose errors.
With all of the modified fields specified with "normal" values, everything was happy and the instance joined my cluster. I checked the config files populated by these values and they looked right.
Also, because I haven't tested this before, I made sure to test a node taint; I was able to see the taint, and then run a pod on it, but only if I specified a toleration.