feat: redefine union request types when c8 REST docs are generated #4363
Conversation
|
👋 🤖 🤔 Hello! Did you make your changes in all the right places? These files were changed only in docs/. You might want to duplicate these changes in versioned_docs/version-8.5/.
You may have done this intentionally, but we wanted to point it out in case you didn't. You can read more about the versioning within our docs in our documentation guidelines. |
pepopowitz
requested review from
jwulf,
akeller,
conceptualshark and
mesellings
pepopowitz
added
theme:api-streamline
|
Thanks for providing this, @pepopowitz 👍 While I agree that the documentation becomes a bit clearer, I struggle with the potential longer-term maintenance costs of this change. The code to introduce this necessarily has to do some text parsing to determine if the change is already present. This in itself usually becomes error-prone quite quickly 😅 Plus, the original spec and the one used and displayed in our reference docs differ quite a bit now. I could imagine that users might be confused if our reference docs look a lot different than a document generated from our public OpenAPI spec. I am wondering if a clear documentation of the endpoint with good descriptions is not good enough in our case and simply creates less maintenance burden to think and take care of. |
I think it depends on how much we anticipate these two endpoints changing. If they are relatively stable APIs, I don't think it would be very risky. If the request shape is still evolving, then I agree that it is risky and fragile. The schema modifications are built in a way that they assume the mutually-exclusive keys are listed first in the schema definition, and that anything below them is shared across the mutually-exclusive request shapes. If the request changes to add more non-key fields, after the key fields, then the modifications should (theoretically) continue to work properly. If the keys themselves change, then yes, the modifications might cause incorrect results. Do you have any thoughts @tmetzke on how stable these specific request schemas are? I'm also open to other suggestions on how to provide this more-accurate schema definition in our docs, because I do think it's important. We've already had two people internally mention that the current definition confused them; I'd expect the confusion to be more wide-spread when the public has access to it. |
|
Can we get to the source of this issue? From this comment:
It sounds as though the core of the issue at the moment is:
This is just me thinking out loud: if I understand the issue, then it seems that the best way to resolve this is to address the complexity at the code generation point, rather than moving it up the stack to the documentation layer, with a complex post-processing patch, or the documentation / support layer by addressing user queries proactively (documentation) and reactively (support). We may not be able to do that before the 8.6.0 release, but could we do that work immediately after? |
…it doesn't match the source exactly.
|
I've heard people's feedback, and I appreciate it, but I would like to move forward with this PR prior to the upcoming release. Re: differences in the spec
Re: introducing risk/fragilityThere is certainly risk and fragility introduced by the parsing/editing in this PR --- but the person who will be affected by this code breaking is me. I am willing to accept that risk, because I think this eliminates more friction than it introduces. Having said all that, I can't merge this without an approval from someone. Well...technically I can, but I would prefer not to. |
|
🧹 Preview environment for this PR has been torn down. |
…4363) * chore: remove unused hack * fix: add missing import * feat: redefine createProcessInstanceRequest to express union type * chore: regenerate docs * feat: redefine evaluateDecisionRequest to express union type * chore: regenerate docs * feat: add a disclaimer to the top of the c8 spec so that people know it doesn't match the source exactly.
Description
Follow-up to #4308 (comment).
Modifies the C8 REST api doc generation strategy to:
Risks
This does introduce drift between the upstream source and the spec in these docs. I personally believe this drift is warranted, because of the improvement to the UX of the generated docs.
The biggest risk with these changes is that the request for these endpoints changes, and the pre-generation steps either fail, or rewrite the spec in a way that doesn't make sense. It'll be me that has to fix things if this happens, and I accept that responsibility.
Other implementations considered
You'll see that the rewriting happens through regular expressions, effectively doing a
string.replace()to the source spec. This results in some tricky code, which I've hopefully commented enough to explain clearly.I did consider using the js-yaml package to update the spec instead of string replacement via regular expression, however it resulted in a lot of churn in the spec due to formatting differences, and worse, it rewrote the
descriptionproperties of many endpoints to line-break in undesired locations. I could live with the churn because once it's committed, it won't re-churn. But the description properties are often intentionally line-broken, at the end of a sentence, because the generator uses the first line formeta descriptiontags, and we don't want those to end mid-sentence.I would reconsider if people think this code is too hard to read or maintain.
Some screenshots of the updated requests
Create process instance, by key:
Create process instance, by ID:
An improvement I wasn't expecting -- previously the the default "example" for these requests was invalid because it contained both key and ID; now it contains only the key, and is valid:
Evaluate decision, by key:
Evaluate decition, by ID:
When should this change go live?
holdlabel or convert to draft PR)PR Checklist
/versioned_docsdirectory./docsdirectory (aka/next/).My changes require an Engineering review, and I've assigned the Engineering DRI or delegate.
My changes require a technical writer review, and I've assigned @camunda/tech-writers as a reviewer.
My changes require a docs infrastructure review.