-
Notifications
You must be signed in to change notification settings - Fork 13
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
PWX-21520 : Add custom namespace during configmap creation #136
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #136 +/- ##
=========================================
+ Coverage 8.06% 8.10% +0.03%
=========================================
Files 18 18
Lines 4686 4664 -22
=========================================
Hits 378 378
+ Misses 4286 4264 -22
Partials 22 22
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
5d1e4e6
to
7da0cce
Compare
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.
If you implement my comment, it'll be a 2 line patch. Why do we need to vendor so many files?
@@ -15,9 +16,9 @@ const ( | |||
// Params is the parameters to use for the Store object | |||
type Params struct { | |||
// Kv is the bootstrap kvdb instance | |||
Kv kvdb.Kvdb | |||
Kv kvdb.Kvdb |
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.
nit: The older formatting looked correct. Could you please keep it aligned like the previous version?
@@ -97,6 +98,7 @@ func GetStoreWithParams( | |||
name string, | |||
lockTryDuration time.Duration, | |||
lockHoldTimeout time.Duration, | |||
nameSpace string, |
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 feel it's not required to pass in namespace as parameter in all these functions.
Why not just call ns := os.Getenv("PX_NAMESPACE")
in newK8sStoreWithParams and pass that to configmap.New().
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.
Actually, why don't we take a step back. Why not call ns := os.Getenv("PX_NAMESPACE")
from sched-ops itself. That way we don't need to pass in the namespace as a parameter to configmap.New?
Had an offline discussion. We want to avoid PX_NAMESPACE usage in all our open-source repos. Nikita will work on removing that from cloud-ops also. |
https://portworx.atlassian.net/browse/PWX-21520
Changes done :
P.S : Need to update cloudops version in "porx" go.mod after this PR is merged.