-
Notifications
You must be signed in to change notification settings - Fork 108
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: expose core db connection settings #1056
Conversation
// +kubebuilder:validation:Optional | ||
// +kubebuilder:validation:Minimum=0 | ||
// +kubebuilder:default=50 | ||
MaxIdleConnections *int32 `json:"maxIdleConnections,omitempty"` |
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.
Why is this set here and not in HarborDatabaseSpec
?
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 passed to Core
and eventually Harbor core deployment to configure the max connections between core
pods and the db, ultimately goes to https://sourcegraph.com/github.com/goharbor/harbor@cbb211e6702517fddcc8b9a7341e8a3479211ca7/-/blob/src/common/dao/pgsql.go?L99.
This is different from the configuring DB's server-side max connections.
Therefore I think this should be in harbor_types instead of in HarborDatabaseSpec
. Thoughts?
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.
Understood after explanation on Slack as well - thank you!
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
Please don't merge yet (hopefully some of the checks will break). It's missing at least edited: nvm just found that |
@MarcelMue @wy65701436 @chlins should we merge it now ? Or do we wait to recreate 1.4 release branch first ? |
I think it's fine to merge now. Also why is CI stuck so weirdly? I am uncertain why github actions are not running. |
I just asked me the same question in another PR, I don't understand neither... @wy65701436 @chlins do you have an idea ? |
Allow to change core.spec.database.maxIdleConnections and core.spec.database.maxOpenConnections in harborcluster. Signed-off-by: Eric Liu <shuaiyi@google.com>
The PR failed at test the following test:
I don't think this failure is related to my change, right? Is it possible to re-run the test or merge it? |
@LiuShuaiyi congrats on merging your first PR here! |
Allow to change core.spec.database.maxIdleConnections and core.spec.database.maxOpenConnections in harborcluster.