-
Notifications
You must be signed in to change notification settings - Fork 531
Add optional ObservedGeneration field to DNSEndpointStatus #270
Add optional ObservedGeneration field to DNSEndpointStatus #270
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shashidharatd The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
||
informers map[reflect.Type]cache.SharedIndexInformer | ||
// startedInformers is used for tracking which informers have been started. | ||
// This allows Start() to be called multiple times safely. | ||
startedInformers map[reflect.Type]bool | ||
} | ||
|
||
// NewSharedInformerFactory constructs a new instance of sharedInformerFactory | ||
// WithCustomResyncConfig sets a custom resync period for the specified informer types. |
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.
Are these new generated changes?
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.
yes, these are the new generated changes. I am using kubebuilder 1.0.3 as in https://github.com/kubernetes-sigs/federation-v2/blob/master/scripts/download-binaries.sh#L44
@@ -413,7 +413,6 @@ var ( | |||
Scope: "Namespaced", | |||
Validation: &v1beta1.CustomResourceValidation{ | |||
OpenAPIV3Schema: &v1beta1.JSONSchemaProps{ | |||
Type: "object", |
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.
Did some changes sneak in unintentionally here? Do we need a new verify task to ensure this doesn't happen again?
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 auto-generated. the code which we have in master probably is generated with older version of kubebuilder.
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.
What version of kubebuilder
did you use to update the generated code?
Looks like an unrelated FederatedDeployment
failure for namespaced e2e tests.
here is the kubebuilder version which i use, its the same which is in
|
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 LGTM. You have some pending comments from @pmorie.
91b04fe
to
86a9e89
Compare
@@ -51,14 +52,18 @@ type DNSEndpointSpec struct { | |||
|
|||
// DNSEndpointStatus defines the observed state of DNSEndpoint | |||
type DNSEndpointStatus struct { | |||
// ObservedGeneration is the generation as observed by the controller consuming the DNSEndpoint. |
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.
Does it make sense to add the link for the discussion here kubernetes-sigs/external-dns#657 (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.
@gyliu513, there is nothing more to describe in the comment. the comment is good enough according to me.
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.
/lgtm
This is according to the discussion in kubernetes-sigs/external-dns#657 (comment)
/cc @pmorie @font