Conversation
patchwork01
left a comment
There was a problem hiding this comment.
I think the idea of a basic endpoint to get the version number makes sense, but you need to decide what you want to implement to start with. You could start with the CDK code or the lambda code.
If you start with the CDK you'll need to focus on the CDK code to deploy API Gateway and hook it up to a lambda, and look up how to do that. You could add it to the cdk module as a new optional stack. The most similar existing code would be WebSocketQueryStack, which does use API Gateway but for a web socket.
If you start with the lambda you'll need to work out how to use the AWS libraries to write a handler that consumes the invocations from API Gateway. You'll need to work out whether it should be a single lambda per endpoint or a lambda that can serve multiple endpoints. The closest code that we've got already would be WebSocketQueryProcessorLambda, but that uses the events for web socket invocations, which is not what you want.
The configuration for how the lambda is invoked by API Gateway will be done in the CDK. I'd consider starting with that.
java/deployment/cdk/src/test/java/sleeper/cdk/stack/RestApiStack.java
Outdated
Show resolved
Hide resolved
|
Stack deployed and queried via curl curl -X POST https://{unique_id}.execute-api.eu-west-2.amazonaws.com/sleeper |
ca61688
left a comment
There was a problem hiding this comment.
1 small comment otherwise looks good to me. It would be great to get a knowledge share on this showing it off
| new IngestBatcherStack(scope, "IngestBatcher", props, coreStacks, ingestStacks); | ||
| } | ||
|
|
||
| // Stack to allow provide a Rest API |
There was a problem hiding this comment.
I'm not sure what this comment means
There was a problem hiding this comment.
Adjust comment for better context
patchwork01
left a comment
There was a problem hiding this comment.
This is looking good now, thanks. The following old comments are unresolved:
java/deployment/rest-api/src/main/java/sleeper/restapi/RestApiLambda.java
Outdated
Show resolved
Hide resolved
java/deployment/cdk/src/main/java/sleeper/cdk/stack/RestApiStack.java
Outdated
Show resolved
Hide resolved
java/deployment/cdk/src/main/java/sleeper/cdk/stack/RestApiStack.java
Outdated
Show resolved
Hide resolved
java/deployment/cdk/src/main/java/sleeper/cdk/stack/RestApiStack.java
Outdated
Show resolved
Hide resolved
java/deployment/cdk/src/main/java/sleeper/cdk/stack/RestApiStack.java
Outdated
Show resolved
Hide resolved
patchwork01
left a comment
There was a problem hiding this comment.
The following old comments are unresolved:
java/deployment/rest-api/src/main/java/sleeper/restapi/RestApiLambda.java
Outdated
Show resolved
Hide resolved
java/deployment/cdk/src/main/java/sleeper/cdk/stack/RestApiStack.java
Outdated
Show resolved
Hide resolved
| public static final PropertyGroup REST_API = instanceGroup("REST API") | ||
| .description("The following instance properties relate to the REST API.") | ||
| .build(); |
There was a problem hiding this comment.
Apologies for not catching this earlier, but I'm not sure there's a reason to make a separate property group just for this? I don't think we anticipate adding a lot of properties for this. We could put it in the common group, at least for now?
| <plugins> | ||
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-shade-plugin</artifactId> |
There was a problem hiding this comment.
It doesn't look like this new fat jar is included in the distribution module. Doesn't that mean the jar won't be copied out to the scripts/jars directory, so it won't be available to be deployed? What did you do when you tested it?
It looks like this needs updating in java/distribution/src/main/assembly/zip.xml?
Make sure you have checked all steps below.
Issue
Feature". Note that before an issue is finished, you can still make a pull request by raising a separate issue
for your progress.
Tests
Documentation
separate issue for that below.