Skip to content

Fix: Alias name validator#7826

Merged
bentsku merged 2 commits intolocalstack:masterfrom
deepak2431:fix-kms-alias_name
Mar 13, 2023
Merged

Fix: Alias name validator#7826
bentsku merged 2 commits intolocalstack:masterfrom
deepak2431:fix-kms-alias_name

Conversation

@deepak2431
Copy link
Contributor

This PR is for the fix of issue #7494
Initially, there was no validation on alias names done to create an alias. I have added the Alias name validation which would raise a ValidationException if the given alias name doesn't start with "alias/"

@bentsku You can review this.

@bentsku bentsku self-requested a review March 9, 2023 17:58
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @deepak2431 and thanks a lot for your contribution!

Would it be possible to add an integration test in tests/integration/test_kms.py?

Here's our documentation about contributing: https://docs.localstack.cloud/contributing/contributing/ and writing integration tests: https://docs.localstack.cloud/contributing/integration-tests/

Thank you!

@deepak2431
Copy link
Contributor Author

deepak2431 commented Mar 10, 2023

Hi @deepak2431 and thanks a lot for your contribution!

Would it be possible to add an integration test in tests/integration/test_kms.py?

Here's our documentation about contributing: https://docs.localstack.cloud/contributing/contributing/ and writing integration tests: https://docs.localstack.cloud/contributing/integration-tests/

Thank you!

Thanks for the review @bentsku.
I have checked it to add integration tests but didn't see any scope. Though, I will check again and see if there's any scope to add it. As the alias name is defined in the fixture only.

@deepak2431
Copy link
Contributor Author

@bentsku I have added the integration test.

Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bentsku I have added the integration test.

Awesome, this is exactly what we needed, thanks a lot for taking the time to write this test!
It's all good to go for me, I'll merge. 👍 congratulations for your first contribution!

@bentsku bentsku merged commit d3a21e8 into localstack:master Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants