Closed erikgb closed 8 months ago
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please ask for approval from erikgb. For more information see the Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
/retest
@erikgb: The following test failed, say /retest
to rerun all failed tests or /retest-required
to rerun all mandatory failed tests:
Test name | Commit | Details | Required | Rerun command |
---|---|---|---|---|
pull-trust-manager-smoke | 37a67b9e9150a7a844f84f6862d42f27dbf14ce7 | link | true | /test pull-trust-manager-smoke |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Adding context to this: checked this PR out locally, then patched the makefile to use the external-applyconfigurations arg first, which produced the following diff:
diff --git a/Makefile b/Makefile
index 98ec359..e580808 100644
--- a/Makefile
+++ b/Makefile
@@ -115,6 +115,7 @@ generate-applyconfigurations: | $(BINDIR)/code-generator-$(CODE_GENERATOR_VERSIO
@echo ">> generating pkg/applyconfigurations..."
$(BINDIR)/code-generator-$(CODE_GENERATOR_VERSION)/applyconfiguration-gen \
--go-header-file hack/boilerplate/boilerplate.go.txt \
+ --external-applyconfigurations k8s.io/apimachinery/pkg/apis/meta/v1.Condition:k8s.io/client-go/applyconfigurations/meta/v1 \
--input-dirs "$(API_DIRS)" \
--output-package "$(GO_MODULE)/pkg/applyconfigurations" \
--trim-path-prefix "$(GO_MODULE)" \
diff --git a/pkg/applyconfigurations/trust/v1alpha1/bundlestatus.go b/pkg/applyconfigurations/trust/v1alpha1/bundlestatus.go
index 4ca4f22..8f86afd 100644
--- a/pkg/applyconfigurations/trust/v1alpha1/bundlestatus.go
+++ b/pkg/applyconfigurations/trust/v1alpha1/bundlestatus.go
@@ -19,14 +19,14 @@ limitations under the License.
package v1alpha1
import (
- v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+ v1 "k8s.io/client-go/applyconfigurations/meta/v1"
)
// BundleStatusApplyConfiguration represents an declarative configuration of the BundleStatus type for use
// with apply.
type BundleStatusApplyConfiguration struct {
- Conditions []v1.Condition `json:"conditions,omitempty"`
- DefaultCAPackageVersion *string `json:"defaultCAVersion,omitempty"`
+ Conditions []v1.ConditionApplyConfiguration `json:"conditions,omitempty"`
+ DefaultCAPackageVersion *string `json:"defaultCAVersion,omitempty"`
}
// BundleStatusApplyConfiguration constructs an declarative configuration of the BundleStatus type for use with
@@ -38,9 +38,12 @@ func BundleStatus() *BundleStatusApplyConfiguration {
// WithConditions adds the given value to the Conditions field in the declarative configuration
// and returns the receiver, so that objects can be build by chaining "With" function invocations.
// If called multiple times, values provided by each call will be appended to the Conditions field.
-func (b *BundleStatusApplyConfiguration) WithConditions(values ...v1.Condition) *BundleStatusApplyConfiguration {
+func (b *BundleStatusApplyConfiguration) WithConditions(values ...*v1.ConditionApplyConfiguration) *BundleStatusApplyConfiguration {
for i := range values {
- b.Conditions = append(b.Conditions, values[i])
+ if values[i] == nil {
+ panic("nil value passed to WithConditions")
+ }
+ b.Conditions = append(b.Conditions, *values[i])
}
return b
}
@SgtCoDFish I think your finding could be very useful in another context, since it allows you to avoid re-generating apply configurations (e.g. generated in another project). But I think the goal here is to make applyconfiguration-gen
treat our own BundleCondition
like the upstream metav1.Condition
- NOT generate an apply configuration struct, but just use the API struct. And I don't see how this flag can be used to achieve that. Am I missing something here? 🤔
Am I missing something here? 🤔
No, you're not! It's my poor communication, sorry; I was working out why it wasn't generating apply configs for the Condition type, and I think I answered that (it's not generating any because it's an external type, I think).
I don't think the flag helps us to achieve your goal. I guess if I'm right about why it's not generating apply configs, we could create a github.com/cert-manager/trust-manager-condition
module containing just the BundleCondition
- but that seems crazy and I don't think we'd actually do that!
Superseded by https://github.com/cert-manager/trust-manager/pull/249.
As discussed in today's stand-up, here is a PR to replace our custom
BundleCondition
with the upstream standardmetav1.Condition
. Only downside of this PR, which might be a reason for not merging it, is the condition API docs that are now inherited from upstream. This goes into the CRD, and IMO the wording was better before. 😆Note to reviewers: I am removing the webhook validation of duplicate bundle conditions, as I think it's not needed anymore. We have set the
+listType
and+listMapKey
markers on the.status.conditions
field, so the api-server should handle eventual duplicates.