Skip to content

Allow ':' in secret option values and reject malformed parameters - #40619

Merged
shunping merged 1 commit into
apache:masterfrom
shunping:secret-manager-11
Oct 15, 2026
Merged

shunping merged 1 commit into
apache:masterfrom
shunping:secret-manager-11

Conversation

@shunping

@shunping shunping commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

In Python's Secret.parse_secret_option, split each parameter on the first ':' only, so values such as ARNs and URLs are kept intact instead of being silently dropped. Java's Secret.parseSecretOption already did this, so no change on that front.

In both SDKs, a parameter without ':' now raises an error instead of being ignored, and empty parameters (e.g. from a trailing ';') are skipped.

In Python's Secret.parse_secret_option, split each parameter on the first
':' only, so values such as ARNs and URLs are kept intact instead of being
silently dropped. Java's Secret.parseSecretOption already did this.

In both SDKs, a parameter without ':' now raises an error instead of being
ignored, and empty parameters (e.g. from a trailing ';') are skipped.
@shunping

shunping commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator Author

r: @stankiewicz since #40579 is a use case of parseSecretOption in Java.

@shunping
shunping marked this pull request as ready for review October 15, 2026 18:03
@shunping
shunping requested a review from stankiewicz October 15, 2026 18:03

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📋 Review Summary

This PR updates Secret.parse_secret_option in Python to split secret parameters on the first : only (matching Java's behavior) so values containing colons such as ARNs and URLs are preserved. It also updates both Java and Python implementations to skip empty parameters (e.g., from trailing semicolons) and raise an exception when a parameter is missing a : separator.

🔍 General Feedback

  • The changes across both Java and Python SDKs are clean, consistent, well-documented, and well-tested with unit tests covering colons in values, trailing semicolons, and malformed parameters.

💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.

@codecov

codecov Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.48%. Comparing base (76f6e6d) to head (1ec167f).
⚠️ Report is 198 commits behind head on master.

Additional details and impacted files
@@              Coverage Diff              @@
##             master   #40619       +/-   ##
=============================================
+ Coverage     56.16%   71.48%   +15.31%     
- Complexity     2288    13984    +11696     
=============================================
  Files          1124     2217     +1093     
  Lines        178336   191020    +12684     
  Branches       1489    11307     +9818     
=============================================
+ Hits         100170   136552    +36382     
+ Misses        75645    50263    -25382     
- Partials       2521     4205     +1684     
Flag Coverage Δ
go ?
java 65.05% <100.00%> (-10.73%) ⬇️
python 79.93% <100.00%> (+0.37%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

Copy link
Copy Markdown
Contributor

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

@shunping

Copy link
Copy Markdown
Collaborator Author

There are compilation failure in workflow PreCommit Java IOs Direct and it is fixed in #40620.

@shunping
shunping merged commit 1a3f101 into apache:master Oct 15, 2026
112 of 116 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants