Repository navigation
Fix EC2 default security group creation when no tags are provided - #487
Open
justinokamoto wants to merge 3 commits into
Open
justinokamoto wants to merge 3 commits into
justinokamoto wants to merge 3 commits into
Conversation
PR dask#471 (dask#471) added a required `tags` parameter to `create_default_security_group` to support tagging resources at creation time. However, it introduced two bugs: 1. The call site in `get_security_group` was not updated to pass `tags`, causing a `TypeError` at runtime whenever EC2Cluster tried to create a default security group: TypeError: create_default_security_group() missing 1 required positional argument: 'tags' 2. Even after passing `tags=None`, the `TagSpecifications` list was always included in the `CreateSecurityGroup` API call — even when the resulting `Tags` list was empty. AWS rejects this with: InvalidParameterValue: Tag specification must have at least one tag Fix 1: Pass `tags=None` from `get_security_group` to `create_default_security_group`, since `get_security_group` has no tags of its own to forward (callers that do, like ECS, pass tags directly and are unaffected). Fix 2: Only include `TagSpecifications` in the API call when there is at least one non-empty tag to attach.
Test collection fails under pytest 9 with: Failed: Marks cannot be applied to fixtures. See docs: https://docs.pytest.org/en/stable/deprecations.html#applying-a-mark-to-a-fixture-function Several fixtures were decorated with `@pytest.mark.external`. pytest has never applied marks on fixtures to the tests that use them (see pytest-dev/pytest#3664), so these marks were silent no-ops. pytest 7.4 started emitting a deprecation warning for this, and pytest 9.0 turned it into a hard error. The CI environments install pytest unpinned, so they picked up the new behavior. Because the marks were no-ops, the conftest `--create-external-resources` gate never applied to tests that only got the mark through a fixture. Those tests were skipped only at runtime by `skip_without_credentials`. Remove the marks from the fixtures and put `@pytest.mark.external` on the tests that use them, where it was missing. This matches the pattern already used in the Nebius and DigitalOcean tests, and makes the external-resource gate work as intended.
Author
|
Including unrelated test fixes so CI passes. Problem was CI doesn't pin pytest and eventually picked up pytest 9.0 which errors when fixtures are marked with |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR #471 (#471) added a required
tagsparameter tocreate_default_security_groupto support tagging resources at creation time. However, it introduced two bugs:The call site in
get_security_groupwas not updated to passtags, causing aTypeErrorat runtime whenever EC2Cluster tried to create a default security group:TypeError: create_default_security_group() missing 1 required
positional argument: 'tags'
Even after passing
tags=None, theTagSpecificationslist was always included in theCreateSecurityGroupAPI call — even when the resultingTagslist was empty. AWS rejects this with:InvalidParameterValue: Tag specification must have at least one tag
Fix 1: Pass
tags=Nonefromget_security_grouptocreate_default_security_group, sinceget_security_grouphas no tags of its own to forward (callers that do, like ECS, pass tags directly and are unaffected).Fix 2: Only include
TagSpecificationsin the API call when there is at least one non-empty tag to attach.