-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Replicator user defined id generator #11626
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
Conversation
a38d58b
to
8be6bd6
Compare
8be6bd6
to
2a01941
Compare
@@ -145,6 +145,7 @@ def apigateway_models_stage_to_json(fn, self): | |||
|
|||
return result | |||
|
|||
# TODO remove this patch when the behavior is implemented in moto |
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.
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.
Quick question as this has been an issue in the past, we should try to prevent -
in custom ids, because it messes up with the regex matching the id (mixing it with the -vpc
suffix or something), and also it should probably be lower case. Where could we implement this?
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.
Thanks for the feedback @bentsku. This is not something that is currently possible. I think it could be done by adding a validator
method to ResourceIdentifier
. By default, in would always return True, but then it can be overriden by specific Indentifier. All it would need then is to call it in find_id_from_sources.
It might be easier to implement in moto to prevent patching. Or we can also patch the specific Identifier we need a validator for and use getattr(identifier, "validator", None)
in LocalstackIdManager.find_id_from_sources
.
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.
Here's how it could look like if we go the patching route 🤔
https://github.com/localstack/localstack/compare/replicator-user-defined-id-generator...custom-id-validator?expand=1
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.
Makes sense, that could be nice! The validator could also log a message, nice that it falls back to the default. We can discuss this, it's currently not the case but sometimes leads to confused users as to why trying to hit their API doesn't match when there's an hyphen.. so thank you for looking into this already!
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.
Yah, I think it could be a nice to have. The logging could be improved I agree. I will keep thinking on a nice solution for this as well, and see if I can come up with a more general solution as I see more use cases.
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.
This looks awesome! 🚀 LGTM for the APIGW part, just missing the API key cleanup, but I'll let others more close to the project approve the PR! Great work! 🥳
@@ -145,6 +145,7 @@ def apigateway_models_stage_to_json(fn, self): | |||
|
|||
return result | |||
|
|||
# TODO remove this patch when the behavior is implemented in moto |
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.
Quick question as this has been an issue in the past, we should try to prevent -
in custom ids, because it messes up with the regex matching the id (mixing it with the -vpc
suffix or something), and also it should probably be lower case. Where could we implement this?
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.
Great 💪
933a29e
to
85db009
Compare
Motivation
As we are starting to build the replicator, we need a mechanism to create resources with custom ids. This pr introduces a set of resource agnostic utils that can be used to handle custom id registration and id generation.
By providing a simple decorator that will return a custom id if one was registered or the result of the decorated function if none are found.
Changes