-
Notifications
You must be signed in to change notification settings - Fork 404
Conversation
Doh just added some crd validation things as well, just hadn't gotten to creating the pr 😅 (#208) |
Thanks, i'll remove my crd changes then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to look more on this but ill post this one comment for now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice. one nit.
thank you for contributing this!
Co-Authored-By: Markus Maga <[email protected]>
Thanks for your comments! I changed the deployment name for more clarity and squashed my changes into a single commit. |
@@ -39,6 +39,7 @@ const systemManagerBackend = new SystemManagerBackend({ | |||
logger | |||
}) | |||
const backends = { | |||
// when adding a new backend, make sure to change the CRD property too |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this comment still valid, or is it an artifact from a previous revision?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its valid :) he had CRD validation in this PR first, but since I had a pr for it as well it was removed from here, but it still applies!
Hey!
I'd like to add e2e tests to the repository in order to test the full path including helm chart configuration, AWS/Localstack integration and the good path (create ES, read data from SSM, wait for secret to appear). This is a first iteration, we can add more tests later on.
I also added validation to the
custom-resource-manifest.json
(using the new.spec
field).Unfortunately, we can't test the STS/assume-role integration - local stack does not seem to support it.