Support custom ids #8216
Support custom ids #8216
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8216 +/- ##
========================================
Coverage 94.53% 94.54%
========================================
Files 1155 1158 +3
Lines 99707 100089 +382
========================================
+ Hits 94261 94629 +368
- Misses 5446 5460 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
moto/utilities/id_generator.py
Outdated
| self._lock = threading.RLock() | ||
| self._id_sources = [] | ||
|
|
||
| self.add_id_source(self.get_custom_id) |
There was a problem hiding this comment.
What's the rationale behind having multiple ID sources?
There was a problem hiding this comment.
Allow for being expanded in the future, maybe adding a deterministic fallback. It might be a bit of a premature optimisation and I am not married to this solution.
There was a problem hiding this comment.
Having a deterministic fallback would make sense here, considering moto_id_generator is the only place where LS can tweak the behaviour.
| deployment_id = create_id() | ||
| # Since there are no unique values to a deployment, we will use the stage name for the deployment. | ||
| # We are also passing a list of deployment ids to generate to prevent overwriting deployments. | ||
| deployment_id = ApigwDeploymentIdentifier( |
There was a problem hiding this comment.
Having a list of ExistingId's essentially means that it's up to the user to call moto_id_manager.set_custom_id(..) before every Deployment is instantiated.
So in pseudo code:
moto_id_manager.set_custom_id(DeploymentIdentifier, 'suffix')
create_stage(name="mystage")
create_deployment() # deployment_id = account.region.suffix.mystage
create_deployment() # deployment_id = random identifier, because 'account.region.suffix.mystage' already exists
moto_id_manager.set_custom_id(DeploymentIdentifier, 'suffix2')
create_deployment() # deployment_id = account.region.suffix2.mystage
Am I understanding the flow correctly?
I can't recall another resource where this is the case, but I'm sure this happens more often.. And it is slightly confusing if the actual behaviour differs for specific Resources.
Do you think it makes sense to implement this in a different way? Maybe make it an incremental generator somehow, so that the names become account.region.deployment1, account.region.deployment2, etc?
There was a problem hiding this comment.
You are correct.
However I don't see this as a big issue but rather a normal behavior.
The way it is currently implemented would guarantee the first run of your IAC will have the same result every time you restart. But subsequent deployment will create a new deployment in a non-destructive manner.
I can see the argument going both ways and if it ever becomes problematic we can review this behaviour, and add an auto-increment.
There was a problem hiding this comment.
LGTM! The actual implementation (writing a ResourceIdentifier class) is fairly simple, and there is plenty of docstrings to understand the actual implementation if people are curious.
There are a few nitpicks, but none of them blocking, so feel fre to address that in a followup PR.
Thank you @cloutierMat!
| existing_ids: ExistingIds = None, | ||
| tags: Tags = None, | ||
| ): | ||
| return GENERATED_ID |
There was a problem hiding this comment.
Bit of a nitpick, and not blocking a merge, but calling this GENERIC_ID may be more fitting, as that's a better opposite to CUSTOM_ID
| @pytest.mark.skipif( | ||
| not settings.TEST_DECORATOR_MODE, reason="Can't access the id manager in proxy mode" | ||
| ) | ||
| def test_create_secret_custom_id(account_id, set_custom_id): |
There was a problem hiding this comment.
Another non-blocking nitpick - the account_id fixture is only useful for tests that will also be run against AWS.
It's faster (and better readable IMO) to use the static ACCOUNT_ID directly.
| def get_custom_id( | ||
| self, resource_identifier: ResourceIdentifier | ||
| ) -> Union[str, None]: | ||
| # retrieves a custom_id for a resource. Returns None |
There was a problem hiding this comment.
| # retrieves a custom_id for a resource. Returns None | |
| # retrieves a custom_id for a resource. Returns None _if_ ... |
| def __init__(self, account_id: str, region: str, name: str): | ||
| super().__init__(account_id, region, name) | ||
|
|
||
| def generate( |
There was a problem hiding this comment.
It would be nice if we could move this method into the ResourceIdentifier-class with some sensible defaults, and only override it if necessary.
It's currently difficult to tell what is sensible/common of course, without implementing many more ResourceIdentifier-classes, so that can wait until we have a better grasp of what identifiers typically look like.
There was a problem hiding this comment.
Good point, I will keep that in mind when expanding support to other resources.
On one side it would make using the generate method a lot simpler if it explicitly defines the property name(s) to pass.
But we will see, it may be that most will require name and will return the same generate_str_id.
ps. I have addressed the remaining comments in #8230
Motivation
This pr aims to introduce a mechanism by which we can define custom ids in order to be able to create a resource with deterministic ids.
A
ResourceIdentifierbase class will allow us to quickly create objects to manage ids. Each resource can implement their owngeneratemethod giving us much flexibility when it comes to the type of id created.The
MotoIdManagerimplements an interface over the registered custom ids. It also contains a methodadd_id_sourcethat will allow us the possibility to further expand the behaviour of the id manager.Changes
ResourceIdentifierinapigatewayandsecretsmanageras to provide examples of how it can be used.Not in scope
Many services can benefit from this upgrade, but only 2 are implemented here to provide a sample of how to use. Follow PRs will spread the functionality to more services. This should help the review process over smaller PRs!