Document feature ask for Azure REST API team #442
Conversation
matthchr
requested review from
babbageclunk,
devigned,
Porges and
theunrepentantgeek
as code owners
matthchr
force-pushed
the
feature/swagger-proposal
branch
from
a3ac70b
to
6fcb282
Compare
Codecov Report
@@ Coverage Diff @@
## master #442 +/- ##
==========================================
+ Coverage 62.08% 62.09% +0.01%
==========================================
Files 159 159
Lines 10594 10606 +12
==========================================
+ Hits 6577 6586 +9
- Misses 3392 3394 +2
- Partials 625 626 +1
Continue to review full report at Codecov.
|
matthchr
force-pushed
the
feature/swagger-proposal
branch
from
8e6a7c8
to
0829ed1
Compare
| The easiest way to accomplish this is just to expose "is this field a reference" as an extension in Swagger using `format`. Probably something like: | ||
| ``` | ||
| "SubResource": { | ||
| "properties": { |
There was a problem hiding this comment.
I would suggest adding common semantic types to the common definitions file. This way we can be even more specific and indicate what kind of resource it is, and not just an arbitrary resource (e.g. indicate that something is a NIC type resource rather than an arbitrary resource type).
There was a problem hiding this comment.
I agree this would be nice -- although I wonder how it plays with how services version. There are multiple references I'm aware of where after-the-fact another type was supported. For example when Shared Image Gallery images came out, I believe you could put those references in the same place the old custom image reference was, with no change to spec or APIVersion. Would the expectation be that the service team would update the spec with that additional information? Just future versions of the spec?
There was a problem hiding this comment.
That is a valid concern. Versioning is hard. But I would also be concerned about adding this kind of support to existing APIs breaking clients regardless of how we model it here - it is not unreasonable that a client would "know" how to follow the reference and get horribly broken if a new, unexpected resource was referenced.
There was a problem hiding this comment.
it is not unreasonable that a client would "know" how to follow the reference and get horribly broken if a new, unexpected resource was referenced.
This is an interesting line of thought - but how could the client follow a particular ARM reference but not follow another... unless there are actually multiple kinds of ARM reference? You'd think that given the lack of information in this proposal, a theoretical client can't really do much except very generic operations on any given reference?
There was a problem hiding this comment.
A client shouldn't do that. But there are plenty of scenarios where you pretty much have to follow links. Not sure if there is a more convenient API these days to list public IP addresses for a VM, but when I last implemented it, I had to follow links. And I'm pretty sure if any of those links started pointing to something vastly different than I expected, my code would have blown up.
So there may not strictly be a formal contract that something will always point to a resource of a given type, but customers will invariably take dependencies on undocumented behavior - and in many cases, that is actually the only way for them to get their workloads to work.
I would expect a change like what you describe above to come with a tail end of ICMs. And it would be difficult to completely blame the caller for having made a mistake/taken a dependency on undocumented behavior.
There was a problem hiding this comment.
Ah yes I was thinking in terms of usage by a generator generating some sort of autogenereated client.
As you say, don't customers have the same problem today already? They realize that a field is a link (we don't document it as such but I'm sure they can identify an ARM ID when they see one), they follow it and expect it to look like X, and then one day it looks like Y and their code blows up?
This proposal doesn't necessarily make that problem any worse than it already is (or does it?)
There was a problem hiding this comment.
The only thing it would make different (I'll leave it up to the reader to determine if it is better or worse) is that we would be more explicit in our contract. Which would make it a contract breaking change if we added support for new resource types being "pointed" to. Which is different than underspecify what it can "point to" (e.g. we didn't tell you exactly what resource type this URL refers to) but users still made assumptions since there was only one type it ever pointed to. And possibly our docs also said it in the description fields. In which case there could be an argument that the change wasn't breaking in the first place, and the caller was the one who made a mistake.
| // Name is the Kubernetes name of the resource. | ||
| Name string `json:"name"` | ||
|
|
||
| // +kubebuilder:validation:Pattern="^/subscriptions/(.+?)/resourcegroups/(.+?)/providers/(.+?)/((.+?)/(.+?))+$" |
There was a problem hiding this comment.
Not sure if this matters for the proposal but just FYI. This isn't the format for all resourceIds. It could be an id of a resource that lives under a subscription so the resourceGroups won't be there, it could be something that lives under a tenant, so subscription and resourcegroups won't be there. it could be an extension resource where you have an entire resourceId in the middle of one of these resourceIds. I believe this also assumes that the end consists of /ref/value/ pairs, but thats not always the case either you can have /ref1/ref2/value/ in fact the number of refs doesn't seem to have a limit.
matthchr
force-pushed
the
feature/swagger-proposal
branch
from
27e7e6c
to
b2a28d4
Compare
No description provided.