Add URL for downloading CLI to the console-config configmap #301

jhadvig · 2019-09-25T19:29:09Z

PR for adding the URL for downloading CLI to the console-config configmap, in clusterInfo.cliDownloadURL field.

Story: https://jira.coreos.com/browse/CONSOLE-1423

openshift-ci-robot · 2019-09-25T19:29:55Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jhadvig
To complete the pull request process, please assign benjaminapetersen
You can assign the PR to them by writing /assign @benjaminapetersen in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

spadgett · 2019-09-25T19:47:01Z

We'll need a way to specify different URLs for different platforms

jhadvig · 2019-09-25T19:51:43Z

@spadgett the downloads pod contains binary for linux,osx, widnows... was in impression that it is the route to the pod that we want to wire to the console.

jhadvig · 2019-09-25T19:52:23Z

Or we want a direct link for each platform in the console's "Command Line Tools" page ?

spadgett · 2019-09-25T20:01:24Z

Or we want a direct link for each platform in the console's "Command Line Tools" page ?

Yes, IMO this is a much better experience than dumping the user in a directory listing.

jhadvig · 2019-09-25T20:10:56Z

@spadgett also, for each platform there are is a: oc, oc.tar, oc.zip
I suppose we want to provide the plane oc binary since its a single file ?

spadgett · 2019-09-25T20:16:25Z

Hm, maybe linux .tar.gz and mac and windows use .zip ?

jhadvig · 2019-09-26T15:38:09Z

@spadgett @benjaminapetersen I've update the PR
Now there will be a URL for each platform we build binary for.

PTAL

jhadvig · 2019-09-26T20:29:19Z

/retest

benjaminapetersen · 2019-09-27T02:27:13Z

pkg/console/subresource/configmap/configmap.go

 	managedConfig *corev1.ConfigMap,
 	infrastructureConfig *configv1.Infrastructure,
-	rt *routev1.Route) (consoleConfigmap *corev1.ConfigMap, unsupportedOverridesHaveMerged bool, err error) {
+	rt *routev1.Route,


Can probably update to

consoleRoute *routev1.Route, downloadsRoute *routev1.Route) (consoleConfigmap *corev1.ConfigMap, unsupportedOverridesHaveMerged bool, err error) {

Mean name both routes now that there are two.

benjaminapetersen · 2019-09-27T02:31:37Z

pkg/console/subresource/configmap/configmap_test.go


 const (
 	host               = "localhost"
+	downloadHost       = "https://downloads-openshift-console.apps.some.cluster.openshift.com"


To be honest, for the test I'd rather inline these. Its a tad repetitious, but I think it makes a better test if the input and output are specific to each test and the output (ie, the want) is hard-coded in the specific test.

Also, it's nice to know that if one test breaks, you can be certain that fixing it won't accidentally break other tests.

It may be good to add one test w/o the downloads route (nil) and make sure the config is still properly generated:

{ name: "Test default configmap, no customization overrides", // downloadsRoute: &routev1.Route{ ... } }, { name: "Test default configmap, without downloads", // downloadsRoute: nil }

It may be good to add one test w/o the downloads route (nil) and make sure the config is still properly generated:

All other TCs have the download route set :)

Also the we have the host const here

benjaminapetersen · 2019-09-27T02:41:51Z

pkg/console/subresource/consoleserver/types.go

+}
+
+// CLIDownloadURLs contains download URLs for each of the platforms we provide CLI binary.
+type CLIDownloadURLs struct {


Maybe simplify this a bit:

type CLIDownloadURLs struct { Linux string `yaml:"linuxDownloadURL"` Mac string `yaml:"macDownloadURL"` Windows string `yaml:"windowsDownloadURL"`

Since its already a heading CLIDownloadURLs we don't need to repeat the DownloadURL part in the field names.

To be honest, we may want to do CLIDownloadURLs[arch][platform] just like the downloads deployment. Only amd64 now.

@benjaminapetersen how about?

type ClusterInfo struct { ConsoleBaseAddress string `yaml:"consoleBaseAddress,omitempty"` ConsoleBasePath string `yaml:"consoleBasePath,omitempty"` MasterPublicURL string `yaml:"masterPublicURL,omitempty"` CLIDownloadURLs *CLIDownloadURLs `yaml:"cliDownloadURLs,omitempty"` } // CLIDownloadURLs contains all the architertures that we are building CLI binaries for. type CLIDownloadURLs struct { Amd64 ArchPlatformsURL `yaml:"amd64,omitempty"` } // ArchPlatformsURL contains URLs for each of the platforms we provide CLI binary. type ArchPlatformsURL struct { Linux string `yaml:"linux"` Mac string `yaml:"mac"` Windows string `yaml:"windows"` }

Although Im not really sure about the CLIDownloadURLs, but its pretty generic and with the aditional architecture specified it kinda dioes make sense?

Yup! Though I wonder if ArchPlatformsURL vs ArchPlatformURLs. As "one platform has many urls", and if you had many archs, you would []ArchPlatformURLs, which have many URLs but are named.

type CLIDownloadURLs struct { Amd64 ArchPlatformURLs `yaml:"amd64,omitempty"` Foo32 ArchPlatformURLs `yaml:"Foo32,omitempty"` Bar64 ArchPlatformURLs `yaml:"Bar64,omitempty"` }

Since we expect 3+ URLs for archs added.

But if we tan to chain it like you mentioned in your previous comment it cant be an array.

+1 on ArchPlatformURLs

benjaminapetersen · 2019-09-27T02:42:50Z

pkg/console/subresource/util/util.go

 	secured := fmt.Sprintf("%s%s", protocol, host)
 	return secured
 }
+


I think you can keep this func in the configmap_builder.go file. It isn't shared elsewhere.

Also, should we give it a unit test?

benjaminapetersen · 2019-09-27T02:45:29Z

pkg/api/api.go

@@ -1,5 +1,12 @@
 package api

+const (


I'm inclined to move these into the configmap_builder.go file, since they are only used in one place.

benjaminapetersen · 2019-09-27T02:48:59Z

pkg/console/operator/sync_v400.go

+		return nil, false, "FailedGetDownloadsRoute", err
+	}
+
+	defaultConfigmap, _, err := configmapsub.DefaultConfigMap(operatorConfig, consoleConfig, managedConfig, infrastructureConfig, rt, downloadsRoute)


want to update the var name to consoleRoute, downloadsRoute?

benjaminapetersen · 2019-09-27T02:58:36Z

pkg/console/subresource/configmap/configmap_test.go

+			},
+		},
+		{
+			name: "Test openshift-console namespace without downloads route",


+1 for a test without the downloads route.
Test openshift-console namespace without downloads route - namespace? I'm a bit confused by the test name, however.

Wasnt really sure how to name the test, other then this since the setup is nil download route. But really should be something like : Test getting URLs for downloading CLI, without downloads route. ... maybe ?

The expectation is that there would be no downloads then, if there is no route, correct? I'm good with long tests names that clearly indicate everything, including expected output.

Perhaps Validate that there is no download CLI links when download route is nil``?

Out test naming convention isn't super strong, so if we have to break it for clarity, thats fine. We could always go back and improve the names of the other tests if you have something that works better.

benjaminapetersen · 2019-09-27T02:59:05Z

pkg/console/subresource/configmap/configmap_test.go

+			},
+		},
+		{
+			name: "Test operator config with Statuspage pageID",


I believe we already had this test, trying to figure out why its showing up as new.

benjaminapetersen · 2019-09-27T02:59:55Z

pkg/console/subresource/consoleserver/config_builder.go

 }
+func (b *ConsoleServerCLIConfigBuilder) CLIDownloadURL(cliDownloadURL string) *ConsoleServerCLIConfigBuilder {
+	if cliDownloadURL != "" {
+		b.cliDownloadURLs = &CLIDownloadURLs{


See my other comment about dropping the DownloadURL redundancy. I think we can clean this up just a bit.

makes sense !

benjaminapetersen

Couple minor things, thx!

jhadvig · 2019-09-27T16:18:43Z

@benjaminapetersen comments addressed. PTAL

jhadvig · 2019-09-29T16:51:15Z

/retest

jhadvig · 2019-09-30T11:06:42Z

@benjaminapetersen so was looking into the console side of this story and figured out that it might be better to create ConsoleCLIDownload CR that will represent the per architecture binaries for all the different platforms.

Eg. I've created following CR :

yaml
apiVersion: console.openshift.io/v1
kind: ConsoleCLIDownload
metadata:
  name: download-cli-amd64
spec:
  description: >
    With the OpenShift command line interface, you can create applications and
    manage OpenShift projects from a terminal.


    The oc binary offers the same capabilities as the kubectl binary, but it is
    further extended to natively support OpenShift Container Platform features.
  displayName: oc - OpenShift Command Line Interface (CLI)  - amd64
  links:
    - href: 'https://www.example.com'
      text: Download oc for Linux
    - href: 'https://www.example.com'
      text: Download oc for Mac
    - href: 'https://www.example.com'
      text: Download oc for Windows

which will produce the third section in the below screenshot:

With this I was thinking that instead of adding logic to operator and console, let the operator create the CR for each arch type, that will contain the binaries for all the platforms.

cc'ing @spadgett for opinion

benjaminapetersen · 2019-09-30T14:31:35Z

Ha, I like this. leverages what we already built, no point in all the additional wiring.

benjaminapetersen · 2019-09-30T14:54:05Z

pkg/console/subresource/configmap/configmap.go


+func getCliDownloadUrl(downloadsRoute *routev1.Route) string {
+	if downloadsRoute != nil {
+		return downloadsRoute.Spec.Host


route.Spec.Host will actually be removed, we have a GetCanonicalHost() func that could be used instead.

spadgett · 2019-09-30T16:37:42Z

If we're going to do this for oc, we might want to do it for odo as well. I'd suggest special handling to make sure oc is moved to the top of the list. We'll need to handle the case where you are running against native k8s and this resource doesn't exist.

Only potential downside I see is we can't give special handling to the different platforms (e.g., highlight the macos link if we detect you are on a mac).

benjaminapetersen · 2019-09-30T20:35:01Z

@spadgett @jhadvig agree we discussed the ordering problem a little bit. ConsoleCLIDownload as a type is pretty specific, We could add an optional platform tag to each link, which would give us the opportunity to have the filtering logic in the console.

benjaminapetersen · 2019-09-30T20:52:24Z

Something like this could be worth discussing:

apiVersion: console.openshift.io/v1
kind: ConsoleCLIDownload
metadata:
  name: example-oc
spec:
  displayName: example-oc
  description: >
    This is a link with platforms
  links:
    - href: 'https://www.example.com'
      name: oc for linux
      platform: linux
    - href: 'https://www.example.com'
      name: oc for mac
      platform: mac
    - href: 'https://www.example.com'
      name: oc for windows
      platform: windows

benjaminapetersen · 2019-10-01T18:45:16Z

/hold

As we investigate this other direction.

jhadvig · 2019-10-03T15:00:23Z

Closing in favour of #306

openshift-ci-robot assigned benjaminapetersen Sep 25, 2019

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 25, 2019

openshift-ci-robot requested review from benjaminapetersen and spadgett September 25, 2019 19:29

jhadvig force-pushed the cli_url branch from 392c19d to cdcc9e8 Compare September 26, 2019 15:34

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 26, 2019

Add URL for downloading CLI to the console-config configmap

a75cd82

jhadvig force-pushed the cli_url branch from cdcc9e8 to a75cd82 Compare September 26, 2019 15:37

benjaminapetersen reviewed Sep 27, 2019

View reviewed changes

benjaminapetersen suggested changes Sep 27, 2019

View reviewed changes

addressing comments

c987bf8

jhadvig force-pushed the cli_url branch from 5c5e1d1 to c987bf8 Compare September 27, 2019 16:11

benjaminapetersen reviewed Sep 30, 2019

View reviewed changes

benjaminapetersen mentioned this pull request Sep 30, 2019

Fix cli-download link by changing "text" to "name" openshift/console#2874

Closed

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2019

jhadvig closed this Oct 3, 2019

Add URL for downloading CLI to the console-config configmap #301

Add URL for downloading CLI to the console-config configmap #301

Uh oh!

Conversation

jhadvig commented Sep 25, 2019

Uh oh!

openshift-ci-robot commented Sep 25, 2019

Uh oh!

spadgett commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhadvig commented Sep 25, 2019

Uh oh!

jhadvig commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spadgett commented Sep 25, 2019

Uh oh!

jhadvig commented Sep 25, 2019

Uh oh!

spadgett commented Sep 25, 2019

Uh oh!

jhadvig commented Sep 26, 2019

Uh oh!

jhadvig commented Sep 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaminapetersen Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhadvig Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaminapetersen Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaminapetersen Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaminapetersen left a comment

Choose a reason for hiding this comment

spadgett commented Sep 25, 2019 •

edited

Loading

jhadvig commented Sep 25, 2019 •

edited

Loading

benjaminapetersen Sep 27, 2019 •

edited

Loading

jhadvig Sep 27, 2019 •

edited

Loading

benjaminapetersen Sep 27, 2019 •

edited

Loading

benjaminapetersen Sep 27, 2019 •

edited

Loading