-
Notifications
You must be signed in to change notification settings - Fork 86
Use namespaces from tenant for Deployments API #2110
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2110 +/- ##
==========================================
+ Coverage 69% 69.06% +0.05%
==========================================
Files 162 162
Lines 15126 15113 -13
==========================================
Hits 10438 10438
+ Misses 3739 3729 -10
+ Partials 949 946 -3
Continue to review full report at Codecov.
|
We should split the Deployment controller services from the WIT. We're building a monolith here. The WIT is supposed to track Work Items, not act as a generic service implementation for all of OSIO. |
@michaelkleinhenz Agreed. We are planning to move some shared utilities to a common repository under fabric8-services (openshiftio/openshift.io#3553), and creating a starter project to facilitate the creation of new backend services (openshiftio/openshift.io#3720). These efforts should make it easier to split deployment services (and possibly others) out of WIT. |
} | ||
|
||
func isInternalNamespace(envType string) bool { | ||
internalTypes := []string{"user", "che", "jenkins"} |
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.
internalTypes
could be a constant or a global variable initialized once for all.
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.
Sure, I have now done this.
for envNS, attr := range up.namespaces { | ||
envName := attr.Type | ||
if envName == nil || len(*envName) == 0 { | ||
return nil, errs.Errorf("namespace %s has no type", envNS) |
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.
should you return an error, or just log the error and keep iterating on the namespaces ?
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 have changed this method to log the error and continue. I have kept the severity as error, let me know if you think this should be lowered.
d780bc0
to
a22a4f3
Compare
@xcoulon is this good now? |
3061c7c
to
dcc9379
Compare
Until now, the Deployments API has used the
fabric8-environments
config map to discover the environments belonging to a user. This config map is generated from a simple template [1], which would miss environments made available to the user via an organization (openshiftio/openshift.io#350).This PR changes the Deployments API to detect environments using namespace information from the User Services controller, which is already used for our existing multi-cluster/proxy support. As a side-effect, this PR should mitigate intermittent issues stemming from this config map not being available (openshiftio/openshift.io#3556).
Fixes: openshiftio/openshift.io#3111
[1] https://github.com/fabric8-services/fabric8-tenant-team/blob/106ef4cb92dee51e0c7d09a5dbd7f96e2c7435f2/packages/fabric8-tenant-team/src/main/fabric8/fabric8-environments-cm.yml