Skip to content
This repository was archived by the owner on Dec 12, 2025. It is now read-only.

Conversation

@chatton
Copy link
Contributor

@chatton chatton commented Sep 7, 2020

All Submissions:

  • Have you signed our CLA?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

This PR introduces a mechanism to allow finer grained status updates.

We will now be able to update arbitrary fields of the status sub resource by creating new status options.

@chatton chatton changed the title Added new statuses and unit tests Allow for arbitrary Status subresource updates Sep 7, 2020
@chatton chatton requested a review from rodrigovalin September 7, 2020 15:19
)

type Option interface {
ApplyOption(mdb *mdbv1.MongoDB)
Copy link
Contributor Author

@chatton chatton Sep 8, 2020

Choose a reason for hiding this comment

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

@rodrigovalin this small signature change lets us not need to worry about tracking the state of a single mdb instance, as we can just modify the one that was passed to this function.

Copy link
Contributor

@rodrigovalin rodrigovalin left a comment

Choose a reason for hiding this comment

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

I think I love the idea you introduced with the different options for the status. I still think we can improve it a bit if we have another pair session. I'm mostly thinking about improving how we use GetResult

// the resource's status is updated to reflect to the state, and any other cleanup
// operators should be performed here
func (r ReplicaSetReconciler) updateAndReturnStatusSuccess(mdb *mdbv1.MongoDB) (mdbv1.MongoDBStatus, error) {
func (r ReplicaSetReconciler) updateStatus(mdb *mdbv1.MongoDB) (reconcile.Result, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function can be removed and just move everything back to Reconcile instead!

If you look closely, this function is only doing another status.Update and simply logging more information about it, nothing else.

The Reconcile function is big enough and everyone will want to make it shorter, but we need to find a reasonable logical boundary, and not just "split it right in the middle"

Copy link
Contributor

@rodrigovalin rodrigovalin left a comment

Choose a reason for hiding this comment

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

LGTM!

@chatton chatton merged commit 7da4f08 into master Sep 9, 2020
@chatton chatton deleted the flexible_reconciliation_results branch September 9, 2020 09:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants