Profiles #98
Profiles #98
Conversation
cpu
left a comment
cpu
left a comment
There was a problem hiding this comment.
Looking good. I had a couple pieces of small feedback to consider.
| Self { | ||
| identifiers, | ||
| replaces: None, | ||
| profile: None, |
There was a problem hiding this comment.
I think similar to ARI we should also gain a profile: Option<String> field on the OrderState.
That would let NewOrder's that don't specify a profile learn what "default" profile the server assigned their order:
If the server is advertizing profiles and receives a newOrder request which does not identify a specific profile, it is RECOMMENDED that the server select a profile and associate it with the new Order object.
Relatedly, the profiles spec doesn't say the server MUST reflect the matching profile for new order requests that do specify a profile. That's probably good feedback to give to the author. I think ideally we would enforce it matches like for ARI.
There was a problem hiding this comment.
The spec isn't specific at all about whether the order state will actually reflect back the selected profile, I think?
There was a problem hiding this comment.
Yeah, I think there's language missing to make that crystal clear. I'm partly inferring it based on the registry update ("ACME Order Object Fields") and the accompanying Pebble logic:
- Here's where it assigns a default if one isn't provided
- And here's where the profile name (provided or the default choice) is populated in the order state
There was a problem hiding this comment.
Also, I couldn't think of a great way to test this.
There was a problem hiding this comment.
Yeah I think the Pebble test code makes it annoying to get at the order state while also using all the shared logic for completing the order. Seems fine to not test it explicitly for now. I think the spec should be updated to be clearer and then we can check it in the lib.rs code.
There was a problem hiding this comment.
The spec isn't specific at all about whether the order state will actually reflect back the selected profile, I think?
@djc It looks like this got addressed in -01 🎉
|
Also maybe worth a README mention as a supported feature? :-) |
No description provided.