deprecation version boundary #5411
Conversation
holmanb
force-pushed
the
holmanb/deprecation-version-boundary
branch
2 times, most recently
from
7b41c13
to
286994a
Compare
holmanb
force-pushed
the
holmanb/deprecation-version-boundary
branch
from
286994a
to
760c85f
Compare
There was a problem hiding this comment.
Looks good some unittest fixes to mock the feature DEPRECATION_INFO_BOUNDARY but otherwise +1
- Would like to see a unit test that exercise a DEPRECATION_INFO_BOUNDARY of something != "devel" to provide examples to other downstream of what version strings appear acceptable and the behavior expected in those cases.
| 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 |
There was a problem hiding this comment.
| 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 |
There was a problem hiding this comment.
I don't see the difference between the suggestion and current values?
cloudinit/features.py
Outdated
| DEPRECATION_INFO_BOUNDARY = "devel" | ||
| """ | ||
| DEPRECATION_INFO_BOUNDARY is used by distros to configure at which version to | ||
| start logging deprecations at a level higher than INFO. |
There was a problem hiding this comment.
Do we want to mention the logs greater than INFO level will generate an exit(2) from cloud-init status
cloudinit/features.py
Outdated
|
|
||
| <value> :: = <default> | <version> | ||
| <default> ::= "devel" | ||
| <version> ::= <major> "." <minor> "." <patch> |
There was a problem hiding this comment.
Patch is optional. Maybe document that aspect? We may want to even limit this to just major.minor as some originally published downstream stable versions of cloud-init may contain versions such as 22.1-14-g2e17a0d6-0ubuntu1~22.04.5. We definitely don't want anything beyond digits and periods.
| <version> ::= <major> "." <minor> "." <patch> | |
| <version> ::= <major> "." <minor> [ "." <patch> ] |
| <version> ::= <major> "." <minor> "." <patch> | |
| <version> ::= <digit> "." <digit> [ "." <digit> ] |
Now that we are accepting a slightly more complex schema than booleans in features.py, I bet if we get more complex values for other 'features', some sort of schema validation of the configured values could be helpful to avoid the potential of downstreams. It's definitely not worth the investment on this PR, but a discussion topic we could bring up if we get more complex configurable feature values.
There was a problem hiding this comment.
Patch is optional. Maybe document that aspect?
+1 Good suggestion
We may want to even limit this to just major.minor as some originally published downstream stable versions of cloud-init may contain versions such as 22.1-14-g2e17a0d6-0ubuntu1~22.04.5
This version doesn't match the documented pattern, and even if a downstream ignores the documentation, Version.from_str() won't accept such a format so they would see the following exception in any tests that they run
>>> util.Version.from_str("24.1-0")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/holmanb/ci-a/cloudinit/util.py", line 3166, in from_str
return cls(*(list(map(int, version.split(".")))))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: invalid literal for int() with base 10: '1-0'
Also, lets say a downstream wants to base their initial release on patch version 24.1.13 and wants to include a deprecation that was added in 24.1.12. Limiting to minor-only versions unnecessarily disallows that.
There was a problem hiding this comment.
Now that we are accepting a slightly more complex schema than booleans in features.py, I bet if we get more complex values for other 'features', some sort of schema validation of the configured values could be helpful to avoid the potential of downstreams. It's definitely not worth the investment on this PR, but a discussion topic we could bring up if we get more complex configurable feature values.
Eh, maybe. Personally I think that trying to prevent errors introduced by downstreams or third-parties in upstream code is a lot lower priority than validating user-introduced errors and documenting the possible cloud.cfg values.
There was a problem hiding this comment.
Patch is optional. Maybe document that aspect?
+1 Good suggestion
FWIW minor is also optional, but I don't think that is worth documenting since it's not likely to be used or useful.
| Version.from_str(features.DEPRECATION_INFO_BOUNDARY) | ||
| ): | ||
| info_deprecations.append( | ||
| SchemaProblem(path, schema_error.message) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 += problemwhich 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:
- less state / context to track -> easier to mentally follow
- easier to refactor into simpler / cleaner code in the future
- less variable names that I need to think up (obviously doesn't apply in this case because pre-existing code)
- 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.
There was a problem hiding this comment.
Good intent/reasoning here thanks.
|
@blackboxsw I just need to add a unittest for this new behavior and then I think it's ready |
Since I added the request for more testing, I figured I'd setup a couple of minor tests to exercise the boundary to get better context on behavior here. Review away, if you like the tests, let's keep em, or of something needs to change go for it. |
blackboxsw
force-pushed
the
holmanb/deprecation-version-boundary
branch
from
a30b048
to
01bc508
Compare
This will all stable distros to define whether a key is deprecated based on the version of cloud-init which deprecated the key.
…#5411) This will all stable distros to define whether a key is deprecated based on the version of cloud-init which deprecated the key.
holmanb
force-pushed
the
holmanb/deprecation-version-boundary
branch
from
048551b
to
7bb1a72
Compare
This will all stable distros to define whether a key is deprecated based on the version of cloud-init which deprecated the key.
This will all stable distros to define whether a key is deprecated based on the version of cloud-init which deprecated the key.
This will all stable distros to define whether a key is deprecated based on the version of cloud-init which deprecated the key.
This will all stable distros to define whether a key is deprecated based on the version of cloud-init which deprecated the key.
Additional Context
Add deprecation version knob for stable release distros. This allows distros to set an "initial release" version for the cloud-init release on a stable distro, and any newly added deprecation in future releases should log at an INFO level.
with the following cloud-config:
this PR yields the following schema annotate output:
and the deprecated key use is logged at level DEPRECATED:
when DEPRECATION_INFO_BOUNDARY is set to 22.2
all deprecations are still reported
however only older deprecations than this version are logged at a
DEPRECATEDlevel:nice to have:
Test Steps
Checklist
Merge type