-
Notifications
You must be signed in to change notification settings - Fork 14
Add generic deprecation warnings code for CRDs to find deprecated fields #1196
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
Conversation
|
Heads up, but using some of the guidance from the test output, I started renaming a couple of fields just to make sure this fully works and the generator picks things up appropriately. You can see some of the changes in 7a100f9 |
RafalKorepta
left a comment
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.
Should the RedpandaConsole need to be changed to DeprecatedRedpandaConsole within Redpanda Spec?
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" |
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.
NIT: The space does not align
| "testing" | |
| "github.com/stretchr/testify/require" | |
| "sigs.k8s.io/controller-runtime/pkg/client" | |
| "testing" | |
| "github.com/stretchr/testify/require" | |
| "sigs.k8s.io/controller-runtime/pkg/client" |
Yeah, eventually, there's still a bunch of fields we should rename for clarity as seen in the generated comment: redpanda-operator/operator/api/redpanda/v1alpha2/zz_generated.deprecations_test.go Lines 12 to 37 in 61e1bc0
but I'm ok with doing that as a follow-up. |
So, the context of this is that we've recently started deprecating a bunch of fields in an attempt to shore up a number of ways we're specifying configuration in the operator. This PR adds another generator that interacts with a package introduced in
pkgfor issuing warnings on using deprecated fields. First the new package:New Package
The
deprecationssubpackage underpkgbasically does a full traversal of aclient.Objectpassed it looking for fields that have names prefixed with the wordDeprecated. If the field is in-use (i.e. it isn't just a zero-value), then we issue a warning due to this being a deprecated field.Generator
The generator basically just generates some tests that attempt to leverage any field determined by the generator to be a "Deprecated" prefixed field and ensures that all of the expected warnings are present when calling
deprecations. FindDeprecatedFieldWarningswith the initialized CRD. In addition it generates a TODO at the top of the generated test file with any fields that it thinks are deprecated, but not currently prefixed with "Deprecated" in the structure.Purpose
This will hopefully help us make use of deprecated fields within the operator noisier so that people will switch, I can imagine rate-limiting output of "hey you're using a deprecated field" on a per-object basis for pretty much all of our CRDs. We could even potentially propagate these in a CRD status field or publish usage statistics if we ever fully do unified instrumentation of the operator.
Eventually I would like to ensure we actually drop usage of our Deprecated fields entirely within the code base so that we can eventually remove them in a version migration down the line. That said, getting some transparency into usage is the first step in that.
Caveat Emptor
The generator itself, as it does somewhat complex ast traversal was partially vibe-coded to save me some time. As it's pretty much only used to generate tests though, I'm personally fine with that for now.