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.

Sign up for GitHub

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

Jump to bottom

feat/330/add destination status #343

Merged
merged 5 commits into from
Feb 3, 2025
Merged

feat/330/add destination status #343

merged 5 commits into from
Feb 3, 2025

Conversation

kirederik
Copy link
Member

this PR closes #330

ChunyiLyu
ChunyiLyu reviewed Jan 31, 2025
View reviewed changes
api/v1alpha1/destination_types.go
@@ -105,13 +105,13 @@ func (d *Destination) GetCleanup() string {

// DestinationStatus defines the observed state of Destination
type DestinationStatus struct {
// INSERT ADDITIONAL STATUS FIELD - define observed state of destination
// Important: Run "make" to regenerate code after modifying this file
Conditions []metav1.Condition `json:"conditions,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

love that we still have scaffolded code from 3 years ago

ChunyiLyu
ChunyiLyu reviewed Jan 31, 2025
View reviewed changes
api/v1alpha1/destination_types.go
// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
// +kubebuilder:resource:scope=Cluster,path=destinations,categories=kratix
// +kubebuilder:printcolumn:name="Ready",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].status`,description="Indicates the destination is ready to use"

Copy link
Member

Choose a reason for hiding this comment

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

nice touch for the additional printed column!

ChunyiLyu
ChunyiLyu approved these changes Jan 31, 2025
View reviewed changes
Copy link
Member

@ChunyiLyu ChunyiLyu left a comment

Choose a reason for hiding this comment

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

LGTM!

The only additional thing is that do you think we could check the existence of statestore in the destination webhook and fail early? Is that beneficial for user experience or does that creates a hard ordering dependency of destination -> statestore.

@kirederik
Copy link
Member Author

The only additional thing is that do you think we could check the existence of statestore in the destination webhook and fail early? Is that beneficial for user experience or does that creates a hard ordering dependency of destination -> statestore.

Good shout... I think i'm more inclined to not force the ordering though; that's would also be consistent with other k8s resources? like if you have a deployment mounting a configmap that does not exist it accepts the creation just fine.

That said, a Warning: state store referenced does not exist wouldn't hurt. Maybe as a separate story?

@ChunyiLyu ChunyiLyu mentioned this pull request Jan 31, 2025
Open
@ChunyiLyu
Copy link
Member

@kirederik a warning sounds like a nice middle ground. Issue: #345

@kirederik kirederik merged commit 089781a into main Feb 3, 2025
6 checks passed
@kirederik kirederik deleted the feat/330/add-destination-status branch February 3, 2025 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChunyiLyu ChunyiLyu ChunyiLyu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: Set status and publish events in Destination to indicate if test files are successfully written
2 participants
@kirederik @ChunyiLyu