Skip to content
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

Enhance implementation of is_comma_delimited_list to fix --services in pods CLI #10142

Merged
merged 2 commits into from Jan 30, 2024

Conversation

whummer
Copy link
Member

@whummer whummer commented Jan 30, 2024

Motivation

Enhance implementation of is_comma_delimited_list(..) to fix --services in pods CLI.

We are currently not accepting dashes in items of comma-separated lists, which is required, e.g., for cognito-idp in the following command:

$ localstack -d pod save --services s3,cognito-idp test-123
...
❌ Error: Input the services as a comma-delimited list

Changes

  • adjust the logic in is_comma_delimited_list(..) to allow passing an item_regex
  • define a default DEFAULT_REGEX_LIST_ITEM regex that allows alphanumeric characters as well as dashes
  • addjust and extend the unit tests

As far as I can see, the util function is currently only used for the pods client logic, so it should not break existing logic in other places.

@whummer whummer requested a review from giograno January 30, 2024 10:48
@whummer whummer added the semver: patch Non-breaking changes which can be included in patch releases label Jan 30, 2024
@coveralls
Copy link

Coverage Status

coverage: 83.827% (+0.002%) from 83.825%
when pulling 2316c1c on comma-list
into 45d39ed on master.

Copy link

LocalStack Community integration with Pro

    2 files      2 suites   1h 21m 24s ⏱️
2 615 tests 2 366 ✅ 249 💤 0 ❌
2 617 runs  2 366 ✅ 251 💤 0 ❌

Results for commit 2316c1c.

Copy link
Member

@giograno giograno left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix @whummer. Was this related to your recent errors when saving a pod?

@whummer
Copy link
Member Author

whummer commented Jan 30, 2024

Was this related to your recent errors when saving a pod?

@giograno Partially, at least this one was reproducible. Will keep digging more and will keep you posted if I discover more issues 👍

@whummer whummer merged commit a7f0f7b into master Jan 30, 2024
27 checks passed
@whummer whummer deleted the comma-list branch January 30, 2024 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants