fix(aws): repair the malformed %s in the tag-key "not permitted" message - #3366
Open
Anai-Guo wants to merge 1 commit into
Open
fix(aws): repair the malformed %s in the tag-key "not permitted" message#3366Anai-Guo wants to merge 1 commit into
Anai-Guo wants to merge 1 commit into
Conversation
`validate_aws_tag` raises for a tag key that does not match `PERMITTED`, but the template reads `Key *s* is not permitted...` instead of `Key *%s* ...`. With only one conversion specifier left for two arguments, the `%` operation raises `TypeError: not all arguments converted during string formatting` before `MetaflowException` is ever constructed. The value branch three lines below is the correct copy (`Value *%s* is not permitted. Tags must match pattern: %s`), as are the two length checks above. Reachable from `--tag` on both AWS Batch (`batch_decorator.py`) and Step Functions (`step_functions_cli.py`): any tag key whose first character is outside `[A-Za-z0-9\s+\-=._:/@]` hits it and the user sees an opaque TypeError instead of the intended message. The existing parametrised test never passes a non-permitted key and swallows bare `Exception`, so the branch was invisible to it; the added test asserts the rendered message for both the key and the value branch. Signed-off-by: Tai An <antai12232931@outlook.com>
Contributor
Greptile SummaryThis PR repairs malformed interpolation in the AWS tag-key validation error and adds focused regression coverage.
Confidence Score: 5/5The PR appears safe to merge; the formatting fix is narrowly scoped and covered by a regression test. The corrected template has one conversion specifier for each supplied argument, preserving validation behavior while restoring the intended exception, and no actionable defects remain.
|
| Filename | Overview |
|---|---|
| metaflow/plugins/aws/aws_utils.py | Corrects the invalid AWS tag-key message so formatting produces the intended MetaflowException. |
| test/unit/test_aws_util.py | Adds regression tests covering exception type and rendered prefixes for invalid tag keys and values. |
Reviews (1): Last reviewed commit: "fix(aws): repair the malformed %s in the..." | Re-trigger Greptile
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
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.
The bug
metaflow/plugins/aws/aws_utils.py::validate_aws_tagrejects a tag key whosecharacters fall outside
PERMITTED, but the message template lost its%:*s*is a literal, so the template has one conversion specifier for twoarguments. The
%operation therefore raisesbefore
MetaflowExceptionis ever constructed — the user never sees themessage the code is trying to give them.
Why this is a typo and not intent
The other three raises in the same function all use
*%s*, and the value branchthree lines below is the exact structural twin of the broken one:
Reachability
validate_aws_tagis called on every user-supplied tag from both AWS backends:metaflow/plugins/aws/batch/batch_decorator.py:245metaflow/plugins/aws/step_functions/step_functions_cli.py:384PERMITTEDis[A-Za-z0-9\s\+\-\=\.\_\:\/\@]and is used withre.match, soany tag key whose first character is outside that class takes the branch —
e.g.
--tag "#team=ml".Verification
Ran
validate_aws_tagextracted verbatim from the file (viaast.get_source_segment, no hand-copying) against a stubMetaflowException,before and after the one-character change:
key="#bad-key", value="ok"TypeError: not all arguments converted during string formattingMetaflowException: Key *#bad-key* is not permitted. Tags must match pattern: [A-Za-z0-9\s\+\-\=\.\_\:\/\@]key="ok", value="#bad-value"MetaflowException: Value *#bad-value* is not permitted. …key="a"*129, value="ok"MetaflowException: Tag key *aaa…* is too long. …key="ok", value="ok"Only the key-not-permitted branch changes.
Why the test suite did not catch it
test/unit/test_aws_util.py::test_validate_aws_tagnever passes anon-permitted key, and it asserts only on a
did_raiseboolean caught from abare
except Exception— aTypeErrorsatisfies it just as well as aMetaflowException. This PR adds a small companion test that asserts therendered message for both the key and the value branch; it fails on
masterwith the
TypeErrorand passes with the fix.black25.12.0 (the version pinned in.pre-commit-config.yaml, with the same-ttarget list) reports both files unchanged.Follow-up, deliberately not in this PR:
re.match(PERMITTED, key)only teststhe first character, so
a#bis accepted. Tightening that to^[...]+$is abehaviour change and belongs in its own PR — happy to open one if you want it.
🤖 Generated with Claude Code