Add api types for console extension resources and console server config #318

benjaminapetersen · 2019-05-16T21:03:10Z

Related to:

Console Operator:
- manifests Add console extension manifests console-operator#229
Console:
- console links Add ConsoleLink CRD to enable custom links in help and user dropdowns console#1360
- console cli downloads Add ConsoleCLIDownload CRD extension console#1441
- console notification banners Add ConsoleNotification CRD that can appear above, below, or above and below the page console#1542

RBAC considerations

Console will directly request console links via something like 'api/kubernetes/apis/console.openshift.io/v1/consolelinks' for all users, this will require RBAC for system:authenticated for this resource. see here
Console will directly request console cli downloads via API for all authenticated users, this will require RBAC for system:authenticated on this resource. see here
Console will directly request console notification banner via API for all authenticated users, this will require RBAC for system:authenticated on this resource. see here

@enj has made the observation that this is config, and as such, only the operator as a privileged component should request it & then pass it to the console pod. The objection to this approach is that every time one of these resources is created, the console pod will need to be restarted. This is problematic as the console in the browser will not be notified & thus the data will not be passed to the user in a timely fashion.

ConsoleServerConfig
In addition, the console server config has previously been approved as an API, but it exists in the console repo. It should likely be migrated & helps clarify the folder structure.

~~Decide if this is how we want to structure this kind of config (new folders under /config) as these are not top-level config. Establishes precedence.~~ Top level /console directory
Generate code
Annotations are still needed in these files (+optional, +required, etc)
gofmt with go1.11 (per travis.yaml)
We need to decide if any of these, such as the banners, ought to be instead handled within the console config (or console operator config).

fyi @spadgett @rhamilto

config/v1/console/extensions/types.go

config/v1/console/types_console_server.go

benjaminapetersen · 2019-05-17T20:21:11Z

@jwforres @bparees for review

console/v1/types_console_notification_banner.go

console/v1/types.go

console/v1/types_console_cli_download.go

console/v1/types_console_link.go

console/v1/types_console_notification_banner.go

console/v1/types_console_cli_download.go

console/v1/types_console_link.go

console/v1/types_console_notification_banner.go

hack/lib/init.sh

console/v1/types_console_link.go

hack/lib/init.sh

benjaminapetersen · 2019-05-21T20:51:36Z

@spadgett also on spec, the manifests have validation for a spec, see here, here, here.

Just a last call confirmation we don't want a spec wrapper. I'll eliminate it from the CRD as well.

bparees · 2019-05-21T20:52:57Z

Just a last call confirmation we don't want a spec wrapper. I'll eliminate it from the CRD as well.

isn't that also going to have ripple effects on other code?

the CRD and the go struct need to be in sync, whatever you do.

spadgett · 2019-05-21T22:42:30Z

It should be pretty straightforward to remove spec in the other code if we don't want it. Nothing has merged yet to console or console-operator.

Is there a reason to keep it? I wouldn't expect any of these resources to ever have a status.

benjaminapetersen · 2019-05-22T19:12:22Z

// With spec
//////////////////////
apiVersion: console.openshift.io/v1
kind: ConsoleCLIDownload
metadata:
  creationTimestamp: '2019-05-22T18:59:36Z'
  generation: 1
  name: example
  resourceVersion: '611639'
  selfLink: /apis/console.openshift.io/v1/consoleclidownloads/example
  uid: be9a861f-7cc3-11e9-8b51-02b1a6acaf5e
spec:
  description: foo bar baz
  displayName: Foo
  link:
    href: 'https://www.redhat.com'
    text: Foo bar

// With no spec
//////////////////////
apiVersion: console.openshift.io/v1
kind: ConsoleCLIDownload
metadata:
  creationTimestamp: '2019-05-22T18:59:36Z'
  generation: 1
  name: example
  resourceVersion: '611639'
  selfLink: /apis/console.openshift.io/v1/consoleclidownloads/example
  uid: be9a861f-7cc3-11e9-8b51-02b1a6acaf5e
description: foo bar baz
displayName: Foo
link:
  href: 'https://www.redhat.com'
  text: Foo bar

I don't know that it hurts us really to keep spec. I guess it means we always have the opportunity to add status, but are not obligated. Would we ever have todo anything like:

reject, too many additional menu items (lots of operators adding, menu gets unwieldy)
reject, too many additional items from a single provider (one operator makes a dozen?)

What is a little more painful about dropping spec is that the properties you care about get alphabetized into the boilerplate of the object. So an object returned from API will look like:

apiVersion: console.openshift.io/v1
description: foo bar baz
displayName: Foo
kind: ConsoleCLIDownload
link:
  href: 'https://www.redhat.com'
  text: Foo bar
metadata:
  creationTimestamp: '2019-05-22T18:59:36Z'
  generation: 1
  name: example
  resourceVersion: '611639'
  selfLink: /apis/console.openshift.io/v1/consoleclidownloads/example
  uid: be9a861f-7cc3-11e9-8b51-02b1a6acaf5e

Or

apiVersion: console.openshift.io/v1
href: 'http://www.redhat.com'
kind: ConsoleLink
location: helpMenu
metadata:
  creationTimestamp: '2019-05-23T17:29:29Z'
  generation: 1
  name: example
  resourceVersion: '60295'
  selfLink: /apis/console.openshift.io/v1/consolelinks/example
  uid: 521f3b4c-7d80-11e9-abc5-067bd6cb3f50
text: My link

I personally find that rather annoying/messy, but I realize its a cosmetic issue, not a functional issue.
This just seems better to me:

apiVersion: console.openshift.io/v1
kind: ConsoleLink
metadata:
  creationTimestamp: '2019-05-23T17:29:29Z'
  generation: 1
  name: example
  resourceVersion: '60295'
  selfLink: /apis/console.openshift.io/v1/consolelinks/example
  uid: 521f3b4c-7d80-11e9-abc5-067bd6cb3f50
spec:
  text: My link
  href: 'https://www.redhat.com'
  location: helpMenu

console/v1/types.go

console/v1/types_console_notification_banner.go

benjaminapetersen · 2019-05-31T20:22:08Z

verify success, better.

console/v1/types_console_cli_download.go

console/v1/types_console_notification.go

spadgett · 2019-06-04T22:40:47Z

lgtm, but verify failed

go build github.com/openshift/api/...
# github.com/openshift/api/console/v1
console/v1/zz_generated.deepcopy.go:234:8: cannot use new(ConsoleLink) (type *ConsoleLink) as type *Link in assignment
make: *** [build] Error 2

benjaminapetersen · 2019-06-05T17:43:46Z

make generate, tiny update pushed, lets see if it passes.

benjaminapetersen · 2019-06-05T19:22:12Z

passed.

spadgett

/lgtm

spadgett · 2019-06-05T19:28:56Z

@bparees ptal, will need your approval

bparees

couple of doc nits

console/v1/types_console_cli_download.go

console/v1/types_console_link.go

console/v1/types_console_notification.go

bparees · 2019-06-05T20:25:44Z

/lgtm cancel
/approve

@spadgett you can reassert your lgtm when the godoc items are addressed.

bparees · 2019-06-05T20:26:40Z

/hold
(in case my lgtm cancel doesn't work)

bparees · 2019-06-05T20:39:44Z

/approve

(prow or github may be having issues....)

Extensions: - console links - console cli downloads - console notification banners

benjaminapetersen · 2019-06-06T14:07:08Z

@spadgett updates made.

spadgett · 2019-06-07T17:16:18Z

/lgtm

openshift-ci-robot · 2019-06-07T17:16:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benjaminapetersen, bparees, spadgett

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [bparees]

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

benjaminapetersen changed the title ~~Add api types for console extension resources and console server config~~ [WIP] Add api types for console extension resources and console server config May 16, 2019

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 16, 2019

openshift-ci-robot requested review from abhinavdahiya and adambkaplan May 16, 2019 21:03

benjaminapetersen commented May 16, 2019

View reviewed changes

config/v1/console/extensions/types.go Outdated Show resolved Hide resolved

benjaminapetersen commented May 16, 2019

View reviewed changes

config/v1/console/types_console_server.go Outdated Show resolved Hide resolved

benjaminapetersen force-pushed the console/extensions branch from e271ded to 86e470e Compare May 20, 2019 15:51

benjaminapetersen commented May 20, 2019

View reviewed changes

console/v1/types_console_notification_banner.go Outdated Show resolved Hide resolved

bparees requested changes May 20, 2019

View reviewed changes

spadgett reviewed May 21, 2019

View reviewed changes

benjaminapetersen force-pushed the console/extensions branch from 1316ca1 to ceac0d6 Compare May 21, 2019 18:26

benjaminapetersen changed the title ~~[WIP] Add api types for console extension resources and console server config~~ Add api types for console extension resources and console server config May 21, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2019

benjaminapetersen force-pushed the console/extensions branch 3 times, most recently from 8128f0b to 769530b Compare May 21, 2019 19:16

benjaminapetersen commented May 21, 2019

View reviewed changes

hack/lib/init.sh Show resolved Hide resolved

benjaminapetersen commented May 21, 2019

View reviewed changes

console/v1/types_console_link.go Outdated Show resolved Hide resolved

spadgett reviewed May 21, 2019

View reviewed changes

console/v1/types_console_link.go Outdated Show resolved Hide resolved

spadgett reviewed May 21, 2019

View reviewed changes

hack/lib/init.sh Outdated Show resolved Hide resolved

benjaminapetersen force-pushed the console/extensions branch from 769530b to 1792e68 Compare May 22, 2019 19:14

benjaminapetersen commented May 22, 2019

View reviewed changes

console/v1/types.go Outdated Show resolved Hide resolved

benjaminapetersen commented May 22, 2019

View reviewed changes

console/v1/types_console_notification_banner.go Outdated Show resolved Hide resolved

benjaminapetersen force-pushed the console/extensions branch from 691badd to 8369852 Compare May 23, 2019 16:49

benjaminapetersen force-pushed the console/extensions branch 2 times, most recently from d8c0f57 to d1686d3 Compare June 3, 2019 17:56

benjaminapetersen commented Jun 3, 2019

View reviewed changes

console/v1/types_console_cli_download.go Outdated Show resolved Hide resolved

spadgett reviewed Jun 3, 2019

View reviewed changes

console/v1/types_console_notification.go Outdated Show resolved Hide resolved

benjaminapetersen force-pushed the console/extensions branch from d1686d3 to a11bac5 Compare June 3, 2019 20:38

benjaminapetersen force-pushed the console/extensions branch from a11bac5 to 0eb8c08 Compare June 5, 2019 17:43

openshift-ci-robot assigned spadgett Jun 5, 2019

spadgett approved these changes Jun 5, 2019

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2019

bparees requested changes Jun 5, 2019

View reviewed changes

openshift-ci-robot assigned bparees Jun 5, 2019

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2019

Add api types for console extension resources and console server config

9eab42c

Extensions: - console links - console cli downloads - console notification banners

benjaminapetersen force-pushed the console/extensions branch from 0eb8c08 to 9eab42c Compare June 6, 2019 14:05

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 7, 2019

openshift-merge-robot merged commit 3ca714b into openshift:master Jun 7, 2019

spadgett mentioned this pull request Jun 7, 2019

Pod Log Links Extension openshift/console#1678

Closed

benjaminapetersen deleted the console/extensions branch June 12, 2019 14:28

Add api types for console extension resources and console server config #318

Add api types for console extension resources and console server config #318

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benjaminapetersen commented May 17, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benjaminapetersen commented May 21, 2019

Uh oh!

bparees commented May 21, 2019

Uh oh!

spadgett commented May 21, 2019

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benjaminapetersen commented May 31, 2019

Uh oh!

Uh oh!

Uh oh!

spadgett commented Jun 4, 2019

Uh oh!

benjaminapetersen commented Jun 5, 2019

Uh oh!

benjaminapetersen commented Jun 5, 2019

Uh oh!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

spadgett commented Jun 5, 2019

Uh oh!

bparees left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bparees commented Jun 5, 2019

Uh oh!

bparees commented Jun 5, 2019

Uh oh!

bparees commented Jun 5, 2019

Uh oh!

benjaminapetersen commented Jun 6, 2019

Uh oh!

spadgett commented Jun 7, 2019

Uh oh!

openshift-ci-robot commented Jun 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

benjaminapetersen commented May 16, 2019 •

edited

Loading

benjaminapetersen commented May 22, 2019 •

edited

Loading