Skip to content

feat(server): make it possible to POST custom typings for testing during typing creation#1602

Open
Vampire wants to merge 2 commits intomainfrom
vampire/custom-types
Open

feat(server): make it possible to POST custom typings for testing during typing creation#1602
Vampire wants to merge 2 commits intomainfrom
vampire/custom-types

Conversation

@Vampire
Copy link
Collaborator

@Vampire Vampire commented Aug 19, 2024

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.pom
then 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.

Copy link
Collaborator Author

Vampire commented Aug 19, 2024

@Vampire Vampire force-pushed the vampire/fix-model-version branch from 2835508 to 2f0d292 Compare August 19, 2024 10:21
@Vampire Vampire force-pushed the vampire/custom-types branch from 77c3c5f to 038cf30 Compare August 19, 2024 10:21
@Vampire Vampire force-pushed the vampire/fix-model-version branch from 2f0d292 to 015701a Compare August 19, 2024 10:50
@Vampire Vampire force-pushed the vampire/custom-types branch from 038cf30 to c318448 Compare August 19, 2024 10:50
@Vampire Vampire force-pushed the vampire/fix-model-version branch from 015701a to d7c77ab Compare August 19, 2024 10:59
@Vampire Vampire force-pushed the vampire/custom-types branch from c318448 to 2d36b76 Compare August 19, 2024 10:59
@Vampire Vampire force-pushed the vampire/fix-model-version branch from d7c77ab to 5e386d6 Compare August 19, 2024 11:48
@Vampire Vampire force-pushed the vampire/custom-types branch from 2d36b76 to 2b90ea5 Compare August 19, 2024 11:48
@Vampire Vampire force-pushed the vampire/fix-model-version branch from 5e386d6 to aaf457e Compare August 19, 2024 18:06
@Vampire Vampire force-pushed the vampire/custom-types branch from 2b90ea5 to e0e929e Compare August 19, 2024 18:09
@Vampire Vampire force-pushed the vampire/fix-model-version branch from aaf457e to 65ffa66 Compare August 20, 2024 10:45
@Vampire Vampire force-pushed the vampire/custom-types branch from e0e929e to 77a6d93 Compare August 20, 2024 10:45
@Vampire Vampire force-pushed the vampire/fix-model-version branch from 65ffa66 to 8c04f7d Compare August 20, 2024 11:26
@Vampire Vampire force-pushed the vampire/custom-types branch 2 times, most recently from 79aa99e to 7fb6fca Compare September 8, 2024 08:10
@Vampire Vampire changed the base branch from main to vampire/unify-coords-name September 8, 2024 08:10
@Vampire Vampire force-pushed the vampire/unify-coords-name branch from 6f967d4 to eace48a Compare September 8, 2024 08:26
@Vampire Vampire force-pushed the vampire/custom-types branch from 7fb6fca to 3fd2df2 Compare September 8, 2024 08:26
@Vampire Vampire force-pushed the vampire/unify-coords-name branch from eace48a to d508bed Compare September 8, 2024 09:01
@Vampire Vampire force-pushed the vampire/custom-types branch from 3fd2df2 to bf416ed Compare September 8, 2024 09:01
@Vampire Vampire force-pushed the vampire/unify-coords-name branch from d508bed to 86848dd Compare September 8, 2024 09:06
@Vampire Vampire force-pushed the vampire/custom-types branch from bf416ed to 5b9b0bd Compare September 8, 2024 09:06
@Vampire Vampire force-pushed the vampire/unify-coords-name branch from 86848dd to 665dcd3 Compare September 8, 2024 20:26
@Vampire Vampire force-pushed the vampire/custom-types branch from 5b9b0bd to 9df9c70 Compare September 8, 2024 20:26
@Vampire Vampire force-pushed the vampire/unify-coords-name branch from 665dcd3 to 6fd7327 Compare September 9, 2024 08:04
@Vampire Vampire force-pushed the vampire/custom-types branch from 9df9c70 to d4821a5 Compare September 9, 2024 08:04
@Vampire Vampire force-pushed the vampire/unify-coords-name branch from 6fd7327 to 02098ce Compare September 9, 2024 10:55
@Vampire Vampire force-pushed the vampire/custom-types branch from d4821a5 to 453cc11 Compare September 9, 2024 10:56
@Vampire Vampire force-pushed the vampire/unify-coords-name branch from 02098ce to 37375d9 Compare September 10, 2024 16:42
@Vampire Vampire force-pushed the vampire/custom-types branch from 453cc11 to 938690e Compare September 10, 2024 16:42
@Vampire Vampire force-pushed the vampire/unify-coords-name branch from 37375d9 to c1dc24a Compare September 23, 2024 08:13
@Vampire Vampire force-pushed the vampire/custom-types branch from 938690e to 6852b0d Compare September 23, 2024 08:14
@Vampire Vampire force-pushed the vampire/unify-coords-name branch from c1dc24a to 6d56a0d Compare September 24, 2024 00:42
@Vampire Vampire force-pushed the vampire/custom-types branch 2 times, most recently from 5340440 to 6d4a4ea Compare September 24, 2024 01:32
@Vampire Vampire force-pushed the vampire/unify-coords-name branch from 6d56a0d to 1dc7506 Compare September 29, 2024 17:28
@Vampire Vampire force-pushed the vampire/custom-types branch from 6d4a4ea to 7cb9a46 Compare September 29, 2024 17:28
Copy link
Member

@krzema12 krzema12 left a comment

Choose a reason for hiding this comment

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

Did the next pass, not fully reviewed. Moderately actionable on you.

val version = call.parameters["version"]!!
val types = call.receiveText()
runCatching {
Load().loadOne(types)
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Comment on lines 26 to +28
fetchUri: (URI) -> String = ::fetchUri,
types: String? = null,
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

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