[Core feature] Enforce Keyword-Only Arguments in SecretsManager.get Method and Add Deprecation Warning

[10sharmashivam]

Tracking issue

Reference Issue

Why are the changes needed?

The changes are needed to enforce keyword-only arguments in the SecretsManager.get method, addressing a common usability issue where users inadvertently use positional arguments, which could lead to confusion or errors in secret retrieval. Additionally, a deprecation warning will be raised whenever positional arguments are passed.

What changes were proposed in this pull request?

1. Updated the SecretsManager.get method to require keyword arguments by adding * to the method signature. This change enforces that all arguments must be provided as keywords.
2. Introduced a deprecation warning for the use of positional arguments. If the method is called with positional arguments, a Deprecation Warning is raised to inform users of the impending removal of support for this usage.

How was this patch tested?

• test_get_with_keyword_arguments: Verified that calling the get method with keyword arguments works as expected.
• test_get_with_positional_arguments: Confirmed that calling the get method with positional arguments raises the appropriate Deprecation Warning.

Check all the applicable boxes

• I updated the documentation accordingly.
• [ X] All new and existing tests passed.
• [ X] All commits are signed off.

[thomasjpfan]

Adding a * right away is backward breaking. This means that old code using position arguments like secret.get("my_group", "key") will stop working.

I implemented a similar deprecation logic in scikit-learn here: https://github.com/scikit-learn/scikit-learn/blob/44017bb4a09359c22d4810c870fd69fbd5673e63/sklearn/utils/validation.py#L33 that you can use in flytekit.

The decorator can be as follows, and it'll raise the correct warning based on where the * is, but also continue to support positional arguments.

@_deprecate_positional_args(version="...")
def get(self, *, ...)

[10sharmashivam]

Hi @thomasjpfan, Thank you for the feedback! I’ll implement the decorator for handling deprecation warnings, similar to the logic used in scikit-learn, to maintain backward compatibility. I’ll update the PR shortly with these changes.

[10sharmashivam]

Hi @thomasjpfan, I’ve updated the PR with the decorator based on the structure and logic from the scikit-learn reference to ensure backward compatibility, issuing a FutureWarning for positional arguments as discussed. While the core logic seems aligned, it didn’t fully pass tests, even with debug statements added for troubleshooting.

Any feedback or guidance on fine-tuning or spotting potential issues with the test outcomes would be a great help!

Regards

[eapolinario]

@10sharmashivam , can you add a few unit tests just to confirm the behavior?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.05%. Comparing base (6bf6f8e) to head (8d5f23f).
Report is 25 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2878       +/-   ##
===========================================
+ Coverage   76.79%   92.05%   +15.25%     
===========================================
  Files         196       28      -168     
  Lines       20546     1598    -18948     
  Branches     2646        0     -2646     
===========================================
- Hits        15779     1471    -14308     
+ Misses       4068      127     -3941     
+ Partials      699        0      -699     

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

[10sharmashivam]

@10sharmashivam , can you add a few unit tests just to confirm the behavior?

Hi @eapolinario

I’ll add the unit tests to confirm the decorator’s behavior and will update the PR soon.

[10sharmashivam]

@10sharmashivam , can you add a few unit tests just to confirm the behavior?

Hi @eapolinario @thomasjpfan,

I’ve implemented the decorator based on the logic and approach suggested by @thomasjpfan, referencing sklearn. The goal is to ensure backward compatibility while issuing a FutureWarning for positional arguments as discussed.

To validate the behavior, I temporarily added debug print statements to _deprecate_positional_args to confirm it is being triggered during tests. I’ve also included unit tests (@eapolinario as suggested), but despite these efforts, the tests are not fully passing, as the FutureWarning is not being captured as expected. I also used the decorator logic in the unit test file, instead of importing it, but still no luck.

Any feedback or guidance on fine-tuning the decorator or identifying potential issues with the tests would be a great help!

Thanks in advance!

Regards