Skip to content
This repository has been archived by the owner on Mar 11, 2021. It is now read-only.

[WIP] migrate WIT to use proxy for all deployments API requests #2284

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

stooke
Copy link
Contributor

@stooke stooke commented Sep 13, 2018

This change removes the "cleverness" in the deployments code (URL provider) to implement multicloster support. Instead, all k8s requests are routed through the OSIO proxy service.

Addresses OSIO issue openshiftio/openshift.io#2096

Once this change is implemented, the deployments API will always need to use the proxy - currently it is optional.

This patch renames some more types that were missed in PR fabric8-services#1868
The /apps API has been replaced by the JSON-API conformant version /deployments.

Fixes fabric8-services#1889
Copy link
Contributor

@ebaron ebaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Simon, this is looking good and it's nice to be able to simplify things. Just a couple comments below.


// GetEnvironmentMapping returns a map whose keys are environment names, and values are the Kubernetes namespaces
// that represent those environments
func (up *tenantURLProvider) GetEnvironmentMapping() map[string]string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will still need this method to translate environment names to Kubernetes namespaces using the information in Tenant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I was hoping to get rid of Tenant in our code, but it seems it's not possible.
I think this API as is is unusable because of the issues @ebaron pointed out, so I propose withdrawing it. The current code will work via the proxy if so configured; this was mainly a cleanup PR. A proper refactoring would leave most of this PR as I have it here (i.e. simplify the URLProvider code), but also add a TenantClient that provides the mapping we reuire from the Tenant service.

var internalNamespaceTypes = map[string]struct{}{"user": {}, "che": {}, "jenkins": {}}

// CanDeploy returns true if the environment type provided can be deployed to as part of a pipeline
func (up *tenantURLProvider) CanDeploy(envType string) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this one too, to filter out environments we want to hide like che and jenkins.

@codecov
Copy link

codecov bot commented Oct 4, 2018

Codecov Report

Merging #2284 into master will increase coverage by 2.21%.
The diff coverage is 57.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2284      +/-   ##
==========================================
+ Coverage   70.13%   72.35%   +2.21%     
==========================================
  Files         174      177       +3     
  Lines       16784    18529    +1745     
==========================================
+ Hits        11771    13406    +1635     
- Misses       3880     3945      +65     
- Partials     1133     1178      +45
Impacted Files Coverage Δ
controller/deployments_urlprovider.go 64% <57.69%> (+16.87%) ⬆️
workitem/typegroup.go 32.17% <0%> (-3.55%) ⬇️
spacetemplate/importer/import_helper.go 55.14% <0%> (-2.12%) ⬇️
spacetemplate/template.go 98% <0%> (-2%) ⬇️
workitem/field_test_data.go 93.82% <0%> (-1.6%) ⬇️
controller/work_item_link_type.go 60.68% <0%> (-1%) ⬇️
workitem/link/link.go 100% <0%> (ø) ⬆️
workitem/link/type.go 100% <0%> (ø) ⬆️
gormsupport/lifecycle.go 100% <0%> (ø) ⬆️
workitem/link/category_repository.go 32.96% <0%> (ø)
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e75e0d1...60a85f1. Read the comment docs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants