Skip to content
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.

Sign up for GitHub

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

Jump to bottom

deprecation version boundary #5411

Merged
merged 3 commits into from
Jun 27, 2024
Merged

deprecation version boundary #5411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d3b61b8
feat: Add deprecation boundary to logger (#5411)
holmanb Jun 18, 2024
a396816
feat: Add deprecation boundary support to schema validator (#5411)
holmanb Jun 19, 2024
7bb1a72
test: Add unit tests for features.DEPRECATION_INFO_BOUNDARY (#5411)
blackboxsw Jun 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 8 additions & 13 deletions cloudinit/cmd/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,17 +236,12 @@ def attempt_cmdline_url(path, network=True, cmdline=None) -> Tuple[int, str]:
is_cloud_cfg = False
if is_cloud_cfg:
if cmdline_name == "url":
return (
log.DEPRECATED,
str(
util.deprecate(
deprecated="The kernel command line key `url`",
deprecated_version="22.3",
extra_message=" Please use `cloud-config-url` "
"kernel command line parameter instead",
return_log=True,
),
),
return util.deprecate(
deprecated="The kernel command line key `url`",
deprecated_version="22.3",
extra_message=" Please use `cloud-config-url` "
"kernel command line parameter instead",
skip_log=True,
)
else:
if cmdline_name == "cloud-config-url":
Expand Down Expand Up @@ -972,8 +967,8 @@ def main(sysv_args=None):
deprecated="`init`",
deprecated_version="24.1",
extra_message="Use `cloud-init init` instead.",
return_log=True,
)
skip_log=True,
).message
parser_mod.add_argument(
"--mode",
"-m",
Expand Down
57 changes: 40 additions & 17 deletions cloudinit/config/schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,14 @@

import yaml

from cloudinit import importer, safeyaml
from cloudinit import features, importer, safeyaml
from cloudinit.cmd.devel import read_cfg_paths
from cloudinit.handlers import INCLUSION_TYPES_MAP, type_from_starts_with
from cloudinit.helpers import Paths
from cloudinit.sources import DataSourceNotFoundException
from cloudinit.temp_utils import mkdtemp
from cloudinit.util import (
Version,
error,
get_modules_from_dir,
load_text_file,
Expand Down Expand Up @@ -137,7 +138,14 @@ class MetaSchema(TypedDict):


class SchemaDeprecationError(ValidationError):
pass
def __init__(
self,
message: str,
version: str,
**kwargs,
):
super().__init__(message, **kwargs)
self.version: str = version


class SchemaProblem(NamedTuple):
Expand Down Expand Up @@ -363,7 +371,7 @@ def _validator(
msg = _add_deprecated_changed_or_new_msg(
schema, annotate=True, filter_key=[filter_key]
)
yield error_type(msg)
yield error_type(msg, schema.get("deprecated_version", "devel"))


_validator_deprecated = partial(_validator, filter_key="deprecated")
Expand Down Expand Up @@ -770,6 +778,7 @@ def validate_cloudconfig_schema(

errors: SchemaProblems = []
deprecations: SchemaProblems = []
info_deprecations: SchemaProblems = []
for schema_error in sorted(
validator.iter_errors(config), key=lambda e: e.path
):
Expand All @@ -785,25 +794,39 @@ def validate_cloudconfig_schema(
)
if prop_match:
path = prop_match["name"]
problem = (SchemaProblem(path, schema_error.message),)
if isinstance(
schema_error, SchemaDeprecationError
): # pylint: disable=W1116
deprecations += problem
if (
"devel" != features.DEPRECATION_INFO_BOUNDARY
and Version.from_str(schema_error.version)
> Version.from_str(features.DEPRECATION_INFO_BOUNDARY)
):
info_deprecations.append(
SchemaProblem(path, schema_error.message)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for my info, what's the perspective on dropping the local var declaration 'problem =' which can be used in both info_deprecations and errors append operations? Just to aid readability at the append call site?

Copy link
Member Author

Choose a reason for hiding this comment

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

The original code created a tuple with a single element and then appended this single element to a list:

problem = (SchemaProblem(path, schema_error.message),)
...
deprecations += problem

which takes some extra mental gymnastics to realize the end goal is to append to a list

deprecations.append(SchemaProblem(path, schema_error.message))

I consider the legibility difference between:

if <blah>:
    info_deprecations.append(SchemaProblem(path, schema_error.message))
else:
    deprecations.append(SchemaProblem(path, schema_error.message))

and

problem = SchemaProblem(path, schema_error.message)
if <blah>:
    info_deprecations.append(problem)
else:
    deprecations.append(problem)

to be negligible. The second has less total calls to grok, but the first has an extra variable to track, and since the calls are simple and identical it's quick realize that they do the same thing.

what's the perspective on dropping the local var declaration 'problem ='

It's really just a style preference. When I find differences like this negligible I typically lean towards the option that involves assigning fewer variables for a few of reasons:

  1. less state / context to track -> easier to mentally follow
  2. easier to refactor into simpler / cleaner code in the future
  3. less variable names that I need to think up (obviously doesn't apply in this case because pre-existing code)
  4. less lines of code to read / maintain

While these reasons reasons seem bit trivial, the impacts add up as code size and complexity grows.

I suppose I could have done this PR without making this change, but the actual reason that this happened due to an earlier iteration subclassing a different type and having to move the assignment within the if isinstance() call which necessitated modifying this variable anyways.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good intent/reasoning here thanks.

)
else:
deprecations.append(SchemaProblem(path, schema_error.message))
else:
errors += problem
errors.append(SchemaProblem(path, schema_error.message))

if log_deprecations and deprecations:
message = _format_schema_problems(
deprecations,
prefix="Deprecated cloud-config provided:\n",
separator="\n",
)
# This warning doesn't fit the standardized util.deprecated() utility
# format, but it is a deprecation log, so log it directly.
LOG.deprecated(message) # type: ignore
if strict and (errors or deprecations):
raise SchemaValidationError(errors, deprecations)
if log_deprecations:
if info_deprecations:
message = _format_schema_problems(
info_deprecations,
prefix="Deprecated cloud-config provided: ",
)
LOG.info(message)
if deprecations:
message = _format_schema_problems(
deprecations,
prefix="Deprecated cloud-config provided: ",
)
# This warning doesn't fit the standardized util.deprecated()
# utility format, but it is a deprecation log, so log it directly.
LOG.deprecated(message) # type: ignore
if strict and (errors or deprecations or info_deprecations):
raise SchemaValidationError(errors, deprecations + info_deprecations)
if errors:
if log_details:
details = _format_schema_problems(
Expand Down
29 changes: 29 additions & 0 deletions cloudinit/features.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,35 @@
to write /etc/apt/sources.list directly.
"""

DEPRECATION_INFO_BOUNDARY = "devel"
"""
DEPRECATION_INFO_BOUNDARY is used by distros to configure at which upstream
version to start logging deprecations at a level higher than INFO.

The default value "devel" tells cloud-init to log all deprecations higher
than INFO. This value may be overriden by downstreams in order to maintain
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
than INFO. This value may be overriden by downstreams in order to maintain
than INFO. This value may be overriden by downstreams in order to maintain

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see the difference between the suggestion and current values?

stable behavior across releases.

Jsonschema key deprecations and inline logger deprecations include a
deprecated_version key. When the variable below is set to a version,
cloud-init will use that version as a demarcation point. Deprecations which
are added after this version will be logged as at an INFO level. Deprecations
which predate this version will be logged at the higher DEPRECATED level.
Downstreams that want stable log behavior may set the variable below to the
first version released in their stable distro. By doing this, they can expect
that newly added deprecations will be logged at INFO level. The implication of
the different log levels is that logs at DEPRECATED level result in a return
code of 2 from `cloud-init status`.

format:

<value> :: = <default> | <version>
<default> ::= "devel"
<version> ::= <major> "." <minor> ["." <patch>]

where <major>, <minor>, and <patch> are positive integers
"""


def get_features() -> Dict[str, bool]:
"""Return a dict of applicable features/overrides and their values."""
Expand Down
35 changes: 25 additions & 10 deletions cloudinit/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
Generator,
List,
Mapping,
NamedTuple,
Optional,
Sequence,
TypeVar,
Expand All @@ -61,6 +62,7 @@
from cloudinit import (
features,
importer,
log,
mergers,
net,
settings,
Expand Down Expand Up @@ -89,6 +91,11 @@
FALSE_STRINGS = ("off", "0", "no", "false")


class DeprecationLog(NamedTuple):
log_level: int
message: str


def kernel_version():
return tuple(map(int, os.uname().release.split(".")[:2]))

Expand Down Expand Up @@ -3209,8 +3216,8 @@ def deprecate(
deprecated_version: str,
extra_message: Optional[str] = None,
schedule: int = 5,
return_log: bool = False,
):
skip_log: bool = False,
) -> DeprecationLog:
"""Mark a "thing" as deprecated. Deduplicated deprecations are
logged.

Expand All @@ -3226,8 +3233,10 @@ def deprecate(
@param schedule: Manually set the deprecation schedule. Defaults to
5 years. Leave a comment explaining your reason for deviation if
setting this value.
@param return_log: Return log text rather than logging it. Useful for
@param skip_log: Return log text rather than logging it. Useful for
running prior to logging setup.
@return: NamedTuple containing log level and log message
DeprecationLog(level: int, message: str)

Note: uses keyword-only arguments to improve legibility
"""
Expand All @@ -3242,14 +3251,20 @@ def deprecate(
f"{deprecated_version} and scheduled to be removed in "
f"{version_removed}. {message}"
).rstrip()
if return_log:
return deprecate_msg
if dedup not in deprecate._log: # type: ignore
if (
holmanb marked this conversation as resolved.
Show resolved Hide resolved
"devel" != features.DEPRECATION_INFO_BOUNDARY
and Version.from_str(features.DEPRECATION_INFO_BOUNDARY) < version
):
LOG.info(deprecate_msg)
level = logging.INFO
elif hasattr(LOG, "deprecated"):
level = log.DEPRECATED
else:
level = logging.WARN
if not skip_log and dedup not in deprecate._log: # type: ignore
deprecate._log.add(dedup) # type: ignore
if hasattr(LOG, "deprecated"):
LOG.deprecated(deprecate_msg) # type: ignore
else:
LOG.warning(deprecate_msg)
LOG.log(level, deprecate_msg)
return DeprecationLog(level, deprecate_msg)


def deprecate_call(
Expand Down
Loading
Loading