PWX-21520 : Add custom namespace during configmap creation #136
PWX-21520 : Add custom namespace during configmap creation #136nikita-bhatia wants to merge 2 commits intomasterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #136 +/- ##
=========================================
- Coverage 8.06% 8.06% -0.01%
=========================================
Files 18 18
Lines 4686 4688 +2
=========================================
Hits 378 378
- Misses 4286 4288 +2
Partials 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nikita-bhatia
force-pushed
the
namespace
branch
from
5d1e4e6 to
7da0cce
Compare
nrevanna
left a comment
nrevanna
left a comment
There was a problem hiding this comment.
If you implement my comment, it'll be a 2 line patch. Why do we need to vendor so many files?
| type Params struct { | ||
| // Kv is the bootstrap kvdb instance | ||
| Kv kvdb.Kvdb | ||
| Kv kvdb.Kvdb |
There was a problem hiding this comment.
nit: The older formatting looked correct. Could you please keep it aligned like the previous version?
| name string, | ||
| lockTryDuration time.Duration, | ||
| lockHoldTimeout time.Duration, | ||
| nameSpace string, |
There was a problem hiding this comment.
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().
nrevanna
left a comment
nrevanna
left a comment
There was a problem hiding this comment.
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. |
nikita-bhatia
changed the title
4 participants
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.