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.

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

feat: Add drift information to the CRP status #913

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

ryanzhang-oss
Copy link
Contributor

Description of your changes

Add drift information to the CRP status

Fixes #

I have:

  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer

Copy link
Contributor

@michaelawyu michaelawyu left a comment

Choose a reason for hiding this comment

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

Just a few nits; otherwise LGTM


// The ObservedGeneration of the resource on the target cluster that caused this drift.
// +kubebuilder:validation:Required
ObservedGeneration int64 `json:"observedGeneration"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Ryan! Just a quick question: this assumes that we will be switching to our own condition implementations to relay this piece of information back, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, considering that observedGeneration is kind of a well-known name in the K8s community, do we want to be more specific that it's about the generation on the target cluster? This is just a nit though, of course.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a random thought, but a driftedSince field could also be useful, just to pinpoint the cause

Copy link
Contributor

Choose a reason for hiding this comment

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

Add observedAt to denote when the observation is made?

// We will also emit an event with the difference details.
// +kubebuilder:validation:maxLength=8192
// +kubebuilder:validation:Optional
DriftDetail string `json:"driftDetail,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I am a bit worried about the size limit as well. Should we mention directly that we might be truncating this information as necessary and the user is expected to check on their own for large diffs?

// +kubebuilder:validation:Required
ResourceIdentifier `json:",inline"`

// The ObservedGeneration of the resource on the target cluster that caused this drift.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The ObservedGeneration of the resource on the target cluster that caused this drift.
// The ObservedGeneration of the resource on the target cluster that contains this drift.


// The ObservedGeneration of the resource on the target cluster that caused this drift.
// +kubebuilder:validation:Required
ObservedGeneration int64 `json:"observedGeneration"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Add observedAt to denote when the observation is made?

apis/placement/v1beta1/clusterresourceplacement_types.go Outdated Show resolved Hide resolved
apis/placement/v1beta1/clusterresourceplacement_types.go Outdated Show resolved Hide resolved
@ryanzhang-oss ryanzhang-oss merged commit 1f26b87 into Azure:main Sep 10, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants