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

poc for aepc #1

Merged
merged 10 commits into from
Oct 4, 2023
Merged

poc for aepc #1

merged 10 commits into from
Oct 4, 2023

Conversation

toumorokoshi
Copy link
Member

@toumorokoshi toumorokoshi commented Sep 15, 2023

barebones poc for aepc, this includes:

  • the resource model, authored as a proto to allow portability in the future.
  • readers for yaml,json, and proto.
  • writer for proto
  • a couple resource examples, with example output.

A lot is missing from this before it's production-ready:

  • support for configurable methods.
  • propagation of field properties.

And at some point deeper testing. But it illustrates the core codebase and resource model. The intention is that we can send PRs to improve the resource model and add functionality along the way.

porting from aep-sandbox.
- applying licenses.
- including output examples.
repeated Resource resources = 2;
}

message Resource {
Copy link
Member Author

Choose a reason for hiding this comment

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

  • parent

Copy link
Collaborator

@rofrankel rofrankel Sep 20, 2023

Choose a reason for hiding this comment

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

Or parents, plural...

Other things you can say about a resource, from the perspective of Google's AIPs:

  • Is it a singleton?
  • Is it versioned?
  • Is it implicit? (that is, does it have a schema at all?)
  • What is the format/pattern of its ID?
  • Which standard fields does it have?

There's also some stuff around naming...depending on what you're generating, you may need both the singular name and the plural name. You might even need some hints about whether and how to abbreviate the collection name (or equivalent path segment for singletons) when it comes to resource names that start with the name of an ancestor resources. For example, if you have DataStore and DataStoreEntry, you might want a URI like data-stores/123/entries/456 rather than data-stores/123/data-store-entries/456. It's possible this can be fully automated, but there might be edge cases that make fully specifying it tough, vs. asking people to provide an abbreviation and then validating it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should fully automate as much as we can - I do accept for adoption purposes we may need to budge on some of the conventions, but I'd like to debate each one a little bit before we add exceptions.

as we do, I think we need a "strict" mode - one where no conventions are broken, and brand new APIs can adhere to them.

We probably want to discuss each of these in individual issues, but:

Is it a singleton?

I've filed #3 to discuss futher.

Is it versioned?

Discuss that in #2?

Is it implicit? (that is, does it have a schema at all?)

Filed #4.

What is the format/pattern of its ID?

You can figure that out by looking at the output: the resource name is {singular}/{id}. We should update the AIPs but I think there should really only be one way to declare a pattern.

Which standard fields does it have?

This is probably an iterative question? the output proto has the standard fields added. You won't find them in the resource definition itself, since they're known fields that all resources have.

There's also some stuff around naming...depending on what you're generating, you may need both the singular name and the plural name.

I dont like the plural name being used in code-generated paths like resource name, but I guess we need it for documentation. I'll add it.

There was a typo in the previous resource definition. Since this will be
user-authored code, adding a validator to ensure valid input.
- adding 'parent' concept in the resource model
- fixing protoc docs
@toumorokoshi toumorokoshi force-pushed the toum/initial-code branch 2 times, most recently from 1316d7d to 1e51ae7 Compare September 27, 2023 15:03
the raw protobuf types aren't enough to help with
things like getting the parent resource structs. Creating
yet another intermediary struct to help facilitate it.

leveraging that functionality to construct paths for
resources with parents.
adding delete method for standard resource types.
resources:
- kind: Book
parents:
- "bookstore.example.com/Publisher"
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: allow a shorthand for intra-service resources (e.g. "Publisher")

Copy link
Member Author

Choose a reason for hiding this comment

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

add a DESIGN.md to include thoughts on each of these topics.

toumorokoshi and others added 4 commits September 27, 2023 21:27
Adding support for relative resource parents, for an slight
ergonomic improvement.

Fixing create path to adhere to the AEPs.
A core requirement for resource-oriented APIs.
since iteration of go maps are not deterministic, some sorting is required
to ensure a proper order.

non-deterministic ordering in proto files may lead to significant
unnescessary diffs if generated protos are checked in.
Adding the ability to set properties in the resource
message, wiring them into the generated resource.
@toumorokoshi toumorokoshi merged commit f237fd2 into aep-dev:main Oct 4, 2023
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.

2 participants