Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Promote AIP-162 to approved #1284

Closed
wants to merge 4 commits into from
Closed

Promote AIP-162 to approved #1284

wants to merge 4 commits into from

Conversation

andrei-scripniciuc
Copy link
Contributor

Summary:

  • Disallow Update standard method, as revisions don't expose mutable fields
  • Fix code samples
  • Promote AIP to approved

There are a few items that still need clarification, as called out in #1206:

  • The user journey of creating a revision with a semantic name is a bit rough: you have to create a revision, then assign it an alias. Ideally it should be one call.
  • Revisions also states that a revision could be a sibling collection instead of a child collection. This pattern is probably possible to support, the AIP needs to be rewritten to allow that (e.g. clarify what the custom method path should be in the case of siblings).
  • How do we allow a single field to address "either a resource or a revision".

These changes can be incrementally added later, moving AIP to approved as we're not planning to address these gaps in the near future.

Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

I'm fine with everything apart from the change to making it approved.

@@ -1,8 +1,8 @@
---
id: 162
state: draft
state: approved
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to separate "we're still making changes" from "it's ready from approval".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps set to reviewing?

Copy link
Collaborator

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

LGTM but if there are things still in flux, let's put into reviewing

@andrei-scripniciuc andrei-scripniciuc closed this by deleting the head repository Feb 29, 2024
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.

3 participants