aws-cloudformation / community-registry-extensions

MIT No Attribution
86 stars 27 forks source link

Add AwsCommunity::Resource::Lookup resource type. #190

Closed mrinaudo-aws closed 1 year ago

mrinaudo-aws commented 1 year ago

Issue #, if available:

Description of changes: Add AwsCommunity::Resource::Lookup resource type.

Tests excerpts

Unit tests excerpts

[...] [INFO] --- jacoco:0.8.9:check (jacoco-check) @ awscommunity-resource-lookup-handler --- [...] [INFO] Analyzed bundle 'awscommunity-resource-lookup-handler' with 10 classes [INFO] All coverage checks have been met. [...]

Contract tests excerpts

[...] 13 passed, 2 skipped, 9 deselected, 1 warning in 1021.84s (0:17:01) [...]

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

benbridts commented 1 year ago

I like this a lot!

A few feature requests:

And some questions related to that:

I think there is also a need for

mrinaudo-aws commented 1 year ago

I like this a lot!

Thank you, @benbridts !

* Can we add a `Serial` or `Trigger` parameter (like [AwsCommunity::Time::Sleep](https://github.com/aws-cloudformation/community-registry-extensions/tree/main/resources/Time_Sleep)), so that you can refresh the value without changing the query (It's not a 100% clear to me from the schema if changing the query triggers a new lookup, so making that more clear might be a good thing too)?

I am currently looking into adding and testing a LookupSerialNumber property in the schema for this.

To your other question, I will clarify in the schema property for JmesPathQuery that when you change the value for the query on e.g., a stack update, a new lookup is invoked.

As per an implementation detail on the schema (related to the previous point), I perform a new lookup via the Create handler even on stack updates: I designed the JmesPathQuery resource property to be a create-only property, as I wanted to provide immutability to the backing SSM parameter, and create a new one if a result is found. I use the Update handler only to update the optional tags for the backing parameter.

* Would `AwsCommunity::Resource::Lookup` be a better name, if that's the only output it can generate?

I chose that name to have a generic and simple one that gives the idea of a lookup without constraining any potential feature updates. The resource description clarifies the current behavior, and with potentially having new behaviors added will be easier and more convenient to update the description than renaming the resource -schema, content, directory structure, URLs).

And some questions related to that:

I think there is also a need for

* a way to get other properties of a resource. This could be part of this  (there could be a `JmesPathSelectorQuery` and a `JmesPathOutputQuery`, or a separate thing (`AwsCommunity::Resource::Read`).

I initially thought of presenting other properties too. Since resources have different properties, it is not possible to statically map them in the schema of this resource unless -for example- you have a new field in the schema where you can add them all (e.g., as a JSON string), and find a way to consume as a resource attribute with a to-be-determined method, or some generic 1:1 mapping like Property1, et cetera. I think your second approach of having a new resource type decouples the lookup logic of this resource from a more specific read operation.

* a way to get a list of resources / identifiers / properties (in that case I think it makes more sense to have a `AwsCommunity::CloudControlQuery::List` and `AwsCommunity::CloudControlQuery::Get` resource, and not the two different kinds of JmesPath). (eg. I want all the subnets of one vpc that have the `type=private` tag).

I think your approach of having 2 other resource types (that go outside the scope for this one) makes sense. I designed this one to keep it relatively simple, and return success if only one result is found (so you can easily consume it without ambiguity); this resource returns a failure if no results are found, or if more than one match is found.

mrinaudo-aws commented 1 year ago
  • Can we add a Serial or Trigger parameter [...]

I am currently looking into adding and testing a LookupSerialNumber property in the schema for this.

Added in a new commit.

mrinaudo-aws commented 1 year ago

[...] (It's not a 100% clear to me from the schema if changing the query triggers a new lookup, so making that more clear might be a good thing too)?

Added this clarification to the description of the relevant property in the schema in the same new commit as well.

mrinaudo-aws commented 1 year ago

Added changes - no code changes made. Contract tests excerpts:

13 passed, 2 skipped, 9 deselected, 1 warning
mrinaudo-aws commented 1 year ago

Small schema and docs updates made. No code changes. Contract tests excerpts:

13 passed, 2 skipped, 9 deselected, 1 warning
mrinaudo-aws commented 1 year ago

Another small documentation-related update made, in the schema. No code changes. Contract tests excerpts:

13 passed, 2 skipped, 9 deselected, 1 warning
mrinaudo-aws commented 1 year ago

Made additional changes to the README.md file. Please review - thanks!

mrinaudo-aws commented 1 year ago

Added changes for using a namespace for the primary ID. Tests excerpts:

[...]
[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running com.awscommunity.resource.lookup.LookupHelperTest
[INFO] Tests run: 20, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.508 s - in com.awscommunity.resource.lookup.LookupHelperTest
[INFO] Running com.awscommunity.resource.lookup.ReadHandlerTest
[INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.779 s - in com.awscommunity.resource.lookup.ReadHandlerTest
[INFO] Running com.awscommunity.resource.lookup.CreateHandlerResourceLookupTest
[INFO] Tests run: 9, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.09 s - in com.awscommunity.resource.lookup.CreateHandlerResourceLookupTest
[INFO] Running com.awscommunity.resource.lookup.CreateHandlerTest
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.037 s - in com.awscommunity.resource.lookup.CreateHandlerTest
[INFO] Running com.awscommunity.resource.lookup.ListHandlerTest
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.03 s - in com.awscommunity.resource.lookup.ListHandlerTest
[INFO] Running com.awscommunity.resource.lookup.UpdateHandlerTest
[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.041 s - in com.awscommunity.resource.lookup.UpdateHandlerTest
[INFO] Running com.awscommunity.resource.lookup.ClientBuilderTest
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.44 s - in com.awscommunity.resource.lookup.ClientBuilderTest
[INFO] Running com.awscommunity.resource.lookup.TagHelperTest
[INFO] Tests run: 16, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.013 s - in com.awscommunity.resource.lookup.TagHelperTest
[INFO] Running com.awscommunity.resource.lookup.CreateHandlerStabilizeTest
[INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.005 s - in com.awscommunity.resource.lookup.CreateHandlerStabilizeTest
[INFO] Running com.awscommunity.resource.lookup.DeleteHandlerTest
[INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.02 s - in com.awscommunity.resource.lookup.DeleteHandlerTest
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 63, Failures: 0, Errors: 0, Skipped: 0
[...]
[...]
INFO] --- jacoco:0.8.9:check (jacoco-check) @ awscommunity-resource-lookup-handler ---
[...]
[INFO] Analyzed bundle 'awscommunity-resource-lookup-handler' with 10 classes
[INFO] All coverage checks have been met.
[...]
13 passed, 2 skipped, 9 deselected, 1 warning