Uh oh!
There was an error while loading. Please reload this page.
feat(server): make it possible to POST custom typings for testing during typing creation - #1602
feat(server): make it possible to POST custom typings for testing during typing creation#1602Vampire wants to merge 3 commits into
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
2835508 to
2f0d292Compare77c3c5f to
038cf30Compare2f0d292 to
015701aCompare038cf30 to
c318448Compare015701a to
d7c77abComparec318448 to
2d36b76Compared7c77ab to
5e386d6Compare2d36b76 to
2b90ea5Compare5e386d6 to
aaf457eCompare2b90ea5 to
e0e929eCompareaaf457e to
65ffa66Comparee0e929e to
77a6d93Compare65ffa66 to
8c04f7dCompare5b9b0bd to
9df9c70Compare665dcd3 to
6fd7327Compare9df9c70 to
d4821a5Compare6fd7327 to
02098ceCompared4821a5 to
453cc11Compare02098ce to
37375d9Compare453cc11 to
938690eCompareUh oh!
There was an error while loading. Please reload this page.
37375d9 to
c1dc24aCompare938690e to
6852b0dComparec1dc24a to
6d56a0dCompare5340440 to
6d4a4eaCompare6d56a0d to
1dc7506Compare6d4a4ea to
7cb9a46Compare
krzema12
left a comment
There was a problem hiding this comment.
Did the next pass, not fully reviewed. Moderately actionable on you.
| val version = call.parameters["version"]!! | ||
| val types = call.receiveText() | ||
| runCatching { | ||
| Load().loadOne(types) |
There was a problem hiding this comment.
How about making toBindingArtifacts communicate such case via its returned value? Then we'd avoid adding a direct dependency on snakeyam-engine-kmp, and generally having parsing these encapsulated in the function (not mentioning it's an abstraction leak because it's not realistic to have anything else than YAML here).
There was a problem hiding this comment.
Actually, I don't really agree.
The value is coming from the HTTP request, so is user input.
User input should always be considered to be not what you expect and be validated.
The body is an "argument" to the "API" and the "API" defines this has to be valid YAML.
So imho it is the right thing to explicitly validate the body to be valid YAML and also do that as soon as possible, not implicitly after several method calls two modules deep.
Also imho doing it that way would be the abstraction leak.
Because currently the place that parses the YAML is not aware that it is run behind an HTTP API.
Doing this change would need to make it aware that it is run behind an HTTP request, because it has to differentiate whether the types are coming as "API argument" so that it can result in "this is an unprocessable entity user error". If any of the other YAMLs has a parsing error, this is not a 4xx user error, as the user calling the HTTP API did not make an error, but it is a 5xx error as something on the server side is not working properly.
| fetchUri: (URI) -> String = ::fetchUri, | ||
| types: String? = null, |
There was a problem hiding this comment.
I think we can make it better - adding types looks like a carveout in this function, and implies some hacks across the project like adding typesUuid to ActionCoords. I'd expect that just parsing of the YAML is triggered if the types as string are provided. I'll think about how to structure it better.
There was a problem hiding this comment.
What do you mean by "carveout"?
It says "give me the types, here is the YAML to use, if I don't give one explicitly default to requesting them from the action or catalog". For me this is not a "carveout" unless I don't get what you mean.
I also do not really see what "hacks" you mean. The "typesUuid" for me is not a hack but a generated part of the coordinates. It also is part of the coordinates, it is just split once and then stored into a separate field.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
With this you can for example do
curl --data-binary @action-types.yml https://bindings.krzeminski.it/pbrisbin/setup-tool-action/v2/setup-tool-action-v2.pomthen get as result coordinates containing a UUID in the group field which you can then use for an hour (or from the Maven Local cache thereafter) to test with
the supplied typing.