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

Replace phases in database status with events #87

Merged
merged 3 commits into from
Jan 24, 2024

Conversation

bobertrublik
Copy link

This PR removes the Phase field from DatabaseStatus and throws Kubernetes Events in its stead. An additional PR is required to remove phases from the CRD in the Helm chart.

Related issue:

Added

I decided to create a local variable var phase containing the current phase in the reconciliation process. This variable is f.e. used during logging or when the function manageError is called.

Removed

The metric promDBsPhase seemed obsolete to me and I removed it. I'm not 100% sure this was correct.

Open for feedback

I wasn't completely sure how to write the event messages and took some liberties there. Maybe someone could provide a few ideas.

@allanger
Copy link
Member

I'm not sure if it actually makes sense, but maybe we can add phase to the context, then we won't need this var, and it should have the same lifettime as reconciliation.
And hopefully, if we later switch to the context logger, we'll be able to always access the phase (I hope)

@bobertrublik
Copy link
Author

bobertrublik commented Dec 20, 2023

It looks like contexts are immutable which means we won't be able to modify the phase each time it changes.

When using contexts, it’s important to know that the values stored in a specific context.Context are immutable, meaning they can’t be changed. When you called the context.WithValue, you passed in the parent context and you also received a context back. You received a context back because the context.WithValue function didn’t modify the context you provided. Instead, it wrapped your parent context inside another one with the new value.

Source

@allanger
Copy link
Member

I'm not sure about metrics, maybe @dabde or @hyunysmile know better.

Copy link
Member

@allanger allanger left a comment

Choose a reason for hiding this comment

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

To be honest, I wouldn't go with the phase variable, because I wanted to unify the manageError method so we don't have it in each controller. Because currently they look pretty much the same and we can use generics or interfaces to get there.

If we go with the phase var, the database's manageError will be different.

Since this phase is a part of the reconciliation, maybe we can put it to the DatabaseReconciler object. Then manageError could access it from the r.DbPhase or something like that

internal/controller/database_controller.go Outdated Show resolved Hide resolved
@allanger
Copy link
Member

To be honest, I wouldn't go with the phase variable, because I wanted to unify the manageError method so we don't have it in each controller. Because currently they look pretty much the same and we can use generics or interfaces to get there.

If we go with the phase var, the database's manageError will be different.

Since this phase is a part of the reconciliation, maybe we can put it to the DatabaseReconciler object. Then manageError could access it from the r.DbPhase or something like that

But maybe nevermind for now, cause I guess my idea will not work that easy anyway. I'm fine with having var for time being

So from my side, only this one: #87 (comment)

Copy link
Member

@allanger allanger left a comment

Choose a reason for hiding this comment

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

I'm approving it. Since metrics are edited in a separate commit, we'll be able to revert it in case we'll turn out to need it

@allanger allanger merged commit 479bf77 into db-operator:main Jan 24, 2024
17 checks passed
@allanger allanger self-assigned this Jan 24, 2024
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.

None yet

2 participants