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

Adopt AIP-0203: Field Behavior #170

Merged
merged 16 commits into from
May 7, 2024
Merged

Conversation

rambleraptor
Copy link
Member

@rambleraptor rambleraptor commented Apr 17, 2024

Fixes #29

Proto changes in aep-dev/aep-components#13

It's unclear to me if we need an OpenAPI extension, but I'd guess we do?

🍱 Types of changes

What types of changes does your code introduce to AEP? Put an x in the boxes
that apply

  • Enhancement
  • New proposal
  • Migrated from google.aip.dev
  • Chore / Quick Fix

📋 Your checklist for this pull request

Please review the AEP Style and Guidance for
contributing to this repository.

General

Additional checklist for a new AEP

  • A new AEP should be no more than two pages if printed out.
  • Ensure that the PR is editable by maintainers.
  • Ensure that File structure
    guidelines are met.
  • Ensure that
    Document structure
    guidelines are met.

💝 Thank you!

@rambleraptor rambleraptor requested a review from a team as a code owner April 17, 2024 20:48
@rambleraptor rambleraptor changed the title adding raw aip Adopt AIP-0203: Field Behavior Apr 17, 2024
@toumorokoshi toumorokoshi self-requested a review April 19, 2024 18:49
@toumorokoshi
Copy link
Member

It's unclear to me if we need an OpenAPI extension, but I'd guess we do?

We definitely need OpenAPI variants - I'll file an issue at least.

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Thanks! A few notes on this one - really on issues in the existing AIPs*.

aep/general/0203/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0203/aep.md.j2 Show resolved Hide resolved
aep/general/0203/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0203/aep.md.j2 Show resolved Hide resolved
aep/general/0203/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0203/aep.md.j2 Show resolved Hide resolved
aep/general/0203/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0203/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0203/aep.md.j2 Outdated Show resolved Hide resolved
rambleraptor and others added 2 commits April 22, 2024 12:22
Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

One last nit, but otherwise LGTM! I'm approving assuming you'll address the one comment, but will hopefully save you the cycle time on approval.

Thanks!

Copy link
Contributor

@mkistler mkistler left a comment

Choose a reason for hiding this comment

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

Need to change "name" to "path" in a bunch of places (I think I caught them all).

After that I think we should merge this. I think this AEP will require extensive changes to generalize the guidance and then add OAS-specifics, so we should do that in a separate PR.

aep/general/0203/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0203/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0203/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0203/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0203/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0203/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0203/aep.md.j2 Outdated Show resolved Hide resolved
@rambleraptor rambleraptor requested a review from mkistler May 4, 2024 17:51
Copy link
Contributor

@mkistler mkistler left a comment

Choose a reason for hiding this comment

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

Two more minor fixes, but I'll approve as these are straightforward.

aep/general/0203/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0203/aep.md.j2 Outdated Show resolved Hide resolved
Co-authored-by: Mike Kistler <[email protected]>
Co-authored-by: Mike Kistler <[email protected]>
@rambleraptor rambleraptor merged commit 8f0330c into aep-dev:main May 7, 2024
2 checks passed
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.

Adopt AIP-0203 Field behavior documentation
3 participants