-
-
Notifications
You must be signed in to change notification settings - Fork 644
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
Deprecate lambdex for building FaaS packages (pending) #19032
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -125,6 +125,9 @@ def complete_platform(rule_runner: PythonRuleRunner) -> bytes: | |
).stdout | ||
|
||
|
||
_first_run_of_deprecated_warning = True | ||
|
||
|
||
@pytest.mark.platform_specific_behavior | ||
@pytest.mark.parametrize( | ||
"major_minor_interpreter", | ||
|
@@ -180,6 +183,15 @@ def handler(event, context): | |
in caplog.text | ||
) | ||
|
||
global _first_run_of_deprecated_warning | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto |
||
if _first_run_of_deprecated_warning: | ||
_first_run_of_deprecated_warning = False | ||
assert ( | ||
'DEPRECATED: use of `layout="lambdex"` (set by default) in target src/python/foo/bar:lambda is scheduled to be removed' | ||
in caplog.text | ||
) | ||
assert "use_deprecated_lambdex_layout" in caplog.text | ||
|
||
|
||
def test_warn_files_targets(rule_runner: PythonRuleRunner, caplog) -> None: | ||
rule_runner.write_files( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1752,6 +1752,20 @@ class GlobalOptions(BootstrapOptions, Subsystem): | |
default=[], | ||
) | ||
|
||
use_deprecated_lambdex_layout = BoolOption( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this a global option? In fact, why do we need this option at all? I might be missing something, but I think it would be simpler to do the relevant check in the code that uses the subsystem, and issue the deprecation warning there? I'm not sure why this is a complicated as it is, so maybe I'm not seeing something. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to allow users (including my work repo) to opt in to the new I suppose another option would be: unconditional (but memoized etc.) warning as it is, and users can add a I'm more than happy to be guided to simpler options, if you think I'm solving problems that don't need to be solved! (Side note, is 2.19 the right release to remove? Maybe it should be a longer deprecation period?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a very low chance that switching from lambdex to zip will have any negative consequence, so I think 2.19.x is fine! But I'm sure we can find a much simpler way of deprecating. For one thing, maybe (and I wish I'd thought of this when reviewing that change, sorry) we don't need a per-target Instead, we can put layout on the Does that make sense? This is often how we do stuff like this - introduce an option to deprecate a feature, then deprecation the option one release later. Either way, this shouldn't be on the global options, because it is very specific to a backend. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, that makes a lot of sense. Simplifying layout to a global option is way better 👍 I had originally been thinking of other layouts that do make sense per-target (e.g. Next steps for me:
Thanks for taking the time to think through a better approach! |
||
default=True, | ||
help=softwrap( | ||
""" | ||
If `true`, any `python_awslambda` and `python_google_cloud_function` targets use the | ||
`layout="lambdex"` by default. This is the older behaviour of these targets, and is | ||
being removed. | ||
If `false`, they will instead use the improved `layout="zip"` by default, which will | ||
become the default in future. | ||
""" | ||
), | ||
) | ||
|
||
@classmethod | ||
def validate_instance(cls, opts): | ||
"""Validates an instance of global options for cases that are not prohibited via | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big code smell, but also we don't typically test deprecation messages, as they are comprehensively tested generically. Manually observing that the right thing happens is sufficient.