fix(ipa): Update IPA to support read-only resources #26
Conversation
yelizhenden-mdb
requested review from
a team,
andmatei,
andreaangiolillo,
drinkbird,
lovisaberggren,
matt-condon and
wtrocki
|
|
||
| Read-only resources are resources that cannot be modified by API consumers. | ||
|
|
||
| - Read-only resources **must** have [Get](0104.md) and [List](0105.md) methods |
There was a problem hiding this comment.
can it be a singleton read-only resource that only has Get but not List?
There was a problem hiding this comment.
Yes, I added read-only singleton resources section to IPA-113: Singleton Resources
| - Read-only resources **must not** have [Create](0106.md), [Update](0107.md), or | ||
| [Delete](0108.md) methods | ||
| - Read-only resources **may** have [custom methods](0109.md) as appropriate | ||
| - All response schema properties for read-only resources **must** be marked as |
There was a problem hiding this comment.
should we also consider create-only / non-updatable resources? (resources that can be created and deleted, but not updated), or other resource patterns, or it's ok at the moment just read-only?
There was a problem hiding this comment.
We may consider create-only resources in the future, but that’s out of scope for this PR. This PR specifically focuses on supporting read-only resources.
As a note for the future: would create-only resources be acceptable as declarative-friendly resources?
| - All fields that are not explicitly documented as server-owned **must** be | ||
| considered client-owned | ||
| - The server **must** respect the value (or lack thereof) of all client-owned | ||
| fields and not modify them |
There was a problem hiding this comment.
I would add something like ... and always return the same value the client sent (or absence of value)
although seems redundant, this is not being respected in current API, where sometimes the default value is returned when client doesn't send the value
| and **must not** be modified by the service | ||
| - A **server-owned** read-only field that records the effective value decided on | ||
| by the service | ||
| - Effective values **must** be named by prefixing `effective` to the mutable |
There was a problem hiding this comment.
it think "must" is strong for prefixing with effective, would use "should", e.g. in some scenarios it can be clear without adding effective
There was a problem hiding this comment.
I think adding a descriptive prefix and forcing it would be helpful here. Since the fields share the same name, a clear prefix would help distinguish ownership. In the following example, both fields are named clusterTier, but one is client-owned and the other is server-owned
There was a problem hiding this comment.
I hear you, just saying that we should use "must" precautiously and I don't think it's so critical here. feel free to keep as it is
There was a problem hiding this comment.
I'd agree with Leo here on adjusting the keyword to should instead of must, I could see use cases where we might want a different name, and AFAIK we don't validate that it's called "effective" nor rely on the naming for downstream tooling
There was a problem hiding this comment.
I’m okay with using either must or should. At a minimum, we could ask for a distinctive prefix so users can clearly understand that the field is effective.
For context, this wording comes from existing guidance — I didn’t introduce any changes here, and I’m not sure if Gus had a strong opinion when it was written. This is just my perspective. I assume he was referring to the AIP guideline here
The reason we don’t validate this is that it’s a semantic concern — we can’t reliably determine programmatically if a field is effective
There was a problem hiding this comment.
Ah I missed this was not an addition! I'm fine to leave it or update, up to you 👍
| [Update](0107.md), or [Delete](0108.md) methods | ||
| - Read-only singleton resources **must not** be able to be modified by the | ||
| client | ||
| - Read-only singleton resources **may** have [custom methods](0109.md) as |
There was a problem hiding this comment.
custom methods are mostly used to cause some effect, is there some example where custom methods would be useful here that don't change anything?
There was a problem hiding this comment.
Good point!
I agree that read-only resources should generally not have custom methods that modify state (as mentioned in this guideline). However, as mentioned in 109, there are no-side-effect custom methods which can be allowed for case-by-case basis—for example, exporting data or analyzing resource state.
We generally don’t recommend custom methods for declarative-friendly resources
AgustinBettati
left a comment
AgustinBettati
left a comment
There was a problem hiding this comment.
leaving some doubts/suggestions (non-blocking)
| fully rely on understandable API responses ([IPA-114](0114.md)) | ||
| - A resource field **should** have a single type ([IPA-125](0125.md)) | ||
|
|
||
| ### Read-Only Resources |
There was a problem hiding this comment.
Placing Read only resource within declarative friendly resources is slightly confusing to me. By definition a declarative friendly resource allows to define what the resource should look like, while the API takes care of the how.
In the case of read only resources they are useful for reading the state of a resource, but provides no management over it. Is there a reason we want to mention it here?
There was a problem hiding this comment.
Yes, IPA-127 has been updated to clarify that read-only resources are considered declarative-friendly. For more details on the restrictions, readers can refer to the references in the Read-Only Resources section.
Are there any concerns regarding the assumption that read-only resources are counted as declarative-friendly?
There was a problem hiding this comment.
No significant concern. This might be more related to how I interpret a declarative-friendly resource:
- (by current definition) resources that can be managed by specifying their desired final state
- as an example, declarative-friendly resources could map to terraform or cloud formation resources. These enable operation which can modify the state of the underlying resource to align with given desired state.
On the other hand read-only resources wont provide any form of management or handling of a desired state. The are only read operations (in terraform world these can be represented as data sources).
There was a problem hiding this comment.
Discussed offline. We have some open questions about whether IPA-127 should define autogeneration safety or cover broader concerns, and whether it would be better to introduce a separate IPA focused specifically on autogeneration safety.
For now, we’re proceeding with merging this PR while we continue to look for a better long-term place for this clarification.
TL;DR: Read-only resources, including read-only singleton resources, are supported by IaC tooling.
|
Validated using #24 |
Update IPA guidelines to support read-only resources
Updated IPAs: