-
Notifications
You must be signed in to change notification settings - Fork 150
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
K8SPSMDB-1256 refactor backup agent (pbm) mongo uri construction #1835
Conversation
pkg/psmdb/statefulset.go
Outdated
@@ -419,7 +425,7 @@ func backupAgentContainer(cr *api.PerconaServerMongoDB, replsetName string, port | |||
}, | |||
{ | |||
Name: "PBM_MONGODB_URI", | |||
Value: "mongodb://$(PBM_AGENT_MONGODB_USERNAME):$(PBM_AGENT_MONGODB_PASSWORD)@$(POD_NAME)", | |||
Value: buildMongoDBURI(ctx, tlsEnabled, sslSecret), |
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.
@gkech, we need to use the new value only in case of
if cr.CompareVersion("1.20.0") >= 0 {
please check the code how we use it
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.
I'll do, thanks Slava
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.
mongoDBURI := "mongodb://$(PBM_AGENT_MONGODB_USERNAME):$(PBM_AGENT_MONGODB_PASSWORD)@$(POD_NAME)" | ||
if cr.CompareVersion("1.20.0") >= 0 { | ||
mongoDBURI = buildMongoDBURI(ctx, tlsEnabled, sslSecret) | ||
} |
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.
it's ok for me and we merge like this, but I prefer to use inverted logic:
mongoDBURI = buildMongoDBURI(ctx, tlsEnabled, sslSecret)
if cr.CompareVersion("1.20.0") < 0 {
mongoDBURI := "mongodb://$(PBM_AGENT_MONGODB_USERNAME):$(PBM_AGENT_MONGODB_PASSWORD)@$(POD_NAME)"
}
It's easier delete unnecessary code when we don't need it.
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.
When it is time to delete code from this implementation, we will have to touch either way the whole block since this part is nested in the if cr.CompareVersion("1.14.0") >= 0
clause. And since we already have this kind of nesting, inverting the logic makes it harder to read because the logical operations are on different direction 😅
I recommend to keep it as is for now and revisit this when it is time to handle old versions.
commit: 1a907df |
CHANGE DESCRIPTION
Problem:
Short explanation of the problem.
Cause:
Short explanation of the root cause of the issue if applicable.
Solution:
Short explanation of the solution we are providing with this PR.
tested image: export IMAGE=perconalab/percona-server-mongodb-operator:K8SPSMDB-1256-9911
CHECKLIST
Jira
Needs Doc
) and QA (Needs QA
)?Tests
compare/*-oc.yml
)?Config/Logging/Testability