Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quick Starts for Console #360

Merged
merged 1 commit into from
Sep 14, 2020

Conversation

jhadvig
Copy link
Member

@jhadvig jhadvig commented Jun 4, 2020

Introducing the Workflow Guides enhancement for the OpenShift Console. The approach is different then the suggested Guided Tours. Main difference is that the Guides are not hosted by the OpenShift platform, but just linked to a GitHub repository. By utilising GitHubs markdown, guide creators will have a proper tool for creating a step by step guide for different area of interests (operators, workloads, ...).

Stories:

@openshift/team-ux-review @spadgett @alimobrem

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 4, 2020
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

Thanks @jhadvig. Since this differs from current designs, we'd need to work with @alimobrem and @openshift/team-ux-review to see if this is an acceptable compromise.

If we're already linking outside of console for these guides, is there a reason we wouldn't link to official OpenShift docs as opposed to a GitHub markdown file?

1. Add additional `ConsoleLinkLocation` type to the `ConsoleLink` [API](https://github.com/openshift/api/blob/master/console/v1/types_console_link.go#L65-L74) named `WorkflowGuide`.
2. If `ConsoleLinkLocation` type is set to `WorkflowGuide`, it will be possible to define a `WorkflowGuideSpec` with following fields:
* Section - string - Defines area of interest, e.g. `operators`.
* Admin - bool - Defines if the Workflow Guide is cluster admin oriented (user has to have cluster admin permisions).
Copy link
Member

Choose a reason for hiding this comment

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

We should be precise about the exact access review we are checking here. We also have environments where users who aren't cluster-admin can still install operators, so this might need to be more nuanced.

Copy link
Member Author

@jhadvig jhadvig Jun 5, 2020

Choose a reason for hiding this comment

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

Thinking about this now it might be a good idea to be have the link agnostic on the user permissions. Since the links will be rendered in:

It shouldn't really matter I think since if user sees the element for starting a particular workflow then he will also see the link for the tour. Otherwise he would only see it in the list of all the Guides.
The Admin field was just for filtering purposes really. meaning which Guides we wanna show when user lists the Guides Dev Console and which for Admin Console, to only show relevant Guides.

Dont think that rendering links for the Guides, either linking to our docs or GH should cause any security issues

2. For linking the OpenShift Console and the Workflow Guides `ConsoleLinks` CRD will be used. To suite the need following extension of the `ConsoleLinks` is needed:
1. Add additional `ConsoleLinkLocation` type to the `ConsoleLink` [API](https://github.com/openshift/api/blob/master/console/v1/types_console_link.go#L65-L74) named `WorkflowGuide`.
2. If `ConsoleLinkLocation` type is set to `WorkflowGuide`, it will be possible to define a `WorkflowGuideSpec` with following fields:
* Section - string - Defines area of interest, e.g. `operators`.
Copy link
Member

Choose a reason for hiding this comment

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

We might want a well-defined list of values and validation to avoid creating separate sections based on small differences like Operators vs operators vs operator.

Copy link
Member Author

@jhadvig jhadvig Jun 5, 2020

Choose a reason for hiding this comment

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

definitely! I was thinking to use the names top section of the left navbar:

  • Operators
  • Workloads
  • Serverless
  • ...
  • User Management
  • Administration
  • Other (?!)


In order for the user to see what resources need to be created in terms of a specific Workflow Guide, we will display list of those resources in a right sidebar similar to the proposed [Solution Explorer](https://marvelapp.com/236ge4ig/screen/69628466). Each of the resource items will be prefixed with an icon which will indicate if the resource exists or not.

For the Resource Check we will need to define a new CRD that will contain list of resources. For each resource, following fields will need to be defined:
Copy link
Member

Choose a reason for hiding this comment

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

I'd inline this directly in WorkflowGuideSpec since presumably this only applies to a single guide.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was thinking about that or maybe create a new CRD that would combine the API of WorkflowGuide and ResourceCheck, but that kinda felt like a dup of the ConsoleLinks so I've decoupled it.
Anyway makes sense 👍

### Implementation Details

1. GitHub will be used creating and storing the Workflow Guides. GitHub provides rich markdown options, including images and videos, which will come handy in the process of creating a Workflow Guide. Since we have storage limits in ETCD, it's for the best to use a technology that both storest the content and provides rich markdown. Also by using GitHub we solve the issue with Workflow Guides versioning.
2. For linking the OpenShift Console and the Workflow Guides `ConsoleLinks` CRD will be used. To suite the need following extension of the `ConsoleLinks` is needed:
Copy link
Member

Choose a reason for hiding this comment

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

Worth noting that ConsoleLink is cluster-scoped, so all guides would be cluster-scoped.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, but as mention above, not sure if thats an issue, since they should be present in wanted elements or the list of Guides

* **Extenstion:**: ResourceCheckName - string - Defines the name of the `ResourceCheck` CR.

3. RedHat supported Workflow Guides will be places in new `openshift/workflow-guides` repository. Each root folder in the repository will be named after area of interest (e.g. top items from the left navigation menu - Operators, Workloads, Serverless, ..., Administration).
4. The `openshift/workflow-guides` repository will also contain a `manifests/` root folder that will contain all yaml for all the official Workflow Guides `ConsoleLinks` CRs. All the manifests will be copied into the `console-operator` image upon it's build. Alternativly there could be a small `workflow-guides-operator` that will be:
Copy link
Member

Choose a reason for hiding this comment

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

All the manifests will be copied into the console-operator image upon it's build.

We probably need to vendor them into the repo for repeatable builds.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes will have to.

Or we could have a dedicated operator that would take care of that and also updates.. and possibly other tasks (TBDefined..)

6. All Workflow Guides will be listed available in a separate page that will be accessible from Help Menu.
![help-menu](https://raw.githubusercontent.com/jhadvig/images/master/help-menu.png)
7. For different areas of interest, the links for Workflow Guides will need to be handles separately:
* Operators - CVS of the operator that has a Workflow Guide published will contain `console.openshift.io/workflow-guide: <console-link-name>` annotation. Upon the operator installation an "Workflow Guide" link will appear next to the `Install` button.
Copy link
Member

Choose a reason for hiding this comment

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

Is the intent for console to list CSVs in the current namespace in addition to ConsoleLink to build one list of guides?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, this is only related to the Operator Hub page where we use the operator's CSV for rendering it in a tile. The CSV could contain mentioned annotation in order to link to a particular ConsoleLink CR.
Im only mention ing the list of the Guides since the UX team was proposing some kind of their list and that could actually be a separate page where user could list though all the Guides

* Kind - string - Kind of the resource.
* Name - string - Name of the resource.
* Namespace - string - Namespace of the resource. (Optional)
* CurrentNamespace - bool - Indicates whether the resource should be check in user's current namespace. If set will be honored instead of the `Namespace` field. (Optional)
Copy link
Member

Choose a reason for hiding this comment

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

Should we just assume the current namespace if no namespace is provided?

Copy link
Member Author

@jhadvig jhadvig Jun 5, 2020

Choose a reason for hiding this comment

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

right but there are also non-namespaced resources, which might need to be referenced.?


#### Air Gapped Environments

If we decide to suppport this case we should have a `workflow-guides-operator`. It's image will contain all the official Workflow Guides. Upon `workflow-guides-operator` image build `openshift/workflow-guides` repository will be cloned into the image so it contains all the official Workflow Guides. The `workflow-guides-operator` will run a pod with a small server that would handle serving of the Workflow Guides.
Copy link
Member

Choose a reason for hiding this comment

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

I don't know that we need to worry about the browser being disconnected in these environments. As I understand, it's requests that come from inside the cluster to the outside that we need to worry about. Browser links to GitHub are probably OK.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, then I guess this is not an issue then

@serenamarie125
Copy link

FYI @christianvogt

@jhadvig
Copy link
Member Author

jhadvig commented Jun 5, 2020

@spadgett

If we're already linking outside of console for these guides, is there a reason we wouldn't link to official OpenShift docs as opposed to a GitHub markdown file?

not at all, we dont put any restrictions on the link URL AFAIK

@jhadvig jhadvig changed the title [WIP] Workflow guides for Console Guided Tours for Console Jun 18, 2020
@openshift-ci-robot 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 Jun 18, 2020
@jhadvig
Copy link
Member Author

jhadvig commented Jun 18, 2020

@spadgett I've update the enhancement. Still finishing the CRD yaml.

@jhadvig
Copy link
Member Author

jhadvig commented Jun 23, 2020

Updated the PR with CRD. Added the "Check your work" feature to both the enhancement doc and CRD.

@spadgett @christianvogt @serenamarie125 @beanh66

@serenamarie125
Copy link

serenamarie125 commented Jun 23, 2020

@jhadvig @christianvogt have you guys thought of how we will determine the featured Tours which are available on the Dev Side?

Also wondering if we are considering "types" - the initial mocks showed blogs in the getting started experience. @alimobrem we should probably connect to see if this is something we'd like to do in the future, so it can be considered early.

@jhadvig
Copy link
Member Author

jhadvig commented Jun 23, 2020

@jhadvig @christianvogt have you guys thought of how we will determine the featured Tours which are available on the Dev Side?

@serenamarie125 makes sense to have an additional field in the spec that would indicate if the tour is meant for the Admin or Dev console. Not sure if it make sense to have a tour in both ?

- displayName
- introduction
- tasks
properties:
Copy link
Contributor

Choose a reason for hiding this comment

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

The tour needs an image property.

about:
description: about introduces the Guided Tour purpose
type: string
usability:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we want more flexibility that comes from a single markdown long description?

@christianvogt
Copy link
Contributor

@jhadvig We're going to need some rbac checks to determine if the tour should be made available to the user or not.

* Introduce CRD format that will be used for writing Guided Tours.
* Introduce default storage for Guided Tours.
* Introduce mechanism how to connect different parts of OpenShift Console (Operators, Workloads, ...) with the Guided Tours.

Copy link

Choose a reason for hiding this comment

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

I am not clear on this goal, how is this goal achieved?

#### Story 4

As a operator creator I want to provide operator consumers with a guide on how to install and user the my operator.

Copy link

Choose a reason for hiding this comment

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

All these stories have different personas repeating the need for a guide. Do we know why these personas need a guide? Is there a UX study that helps understand why our current UI can not guide them through? Have we exhausted the easier/cheaper alternates to fix it on our UIs?

Copy link
Member Author

Choose a reason for hiding this comment

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

All these stories have different personas repeating the need for a guide. Do we know why these personas need a guide? Is there a UX study that helps understand why our current UI can not guide them through?

I think this is a question to @beanh66

Have we exhausted the easier/cheaper alternates to fix it on our UIs?

yes, my first suggestion was to store the GuidedTours in a single GitHub repository (eg. openshfit/guieded-tours) and use the ConsoleLinks to link particular Console's workflows to them.
Other option was to have the GuidedTours in OpenShift's docs and use iframe to show them in the Console, but we decided not to go that road

that is offered to the user by completing the Guided Tour
type: string
tasks:
type: array
Copy link

Choose a reason for hiding this comment

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

I am not sure why tasks needs to be this structured. Each task could essentially be a single markdown. There is no automated checks, execution of tasks, progress monitoring. I can not think of a reason why we need this structure other than some marginal control over rendering.

It would have been great if we could have more improved capabilities like ability to link to parts of console, copy commands. Define criteria to follow progress.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah makes sense to drop the structuring and keep it simple

@serenamarie125
Copy link

@serenamarie125 makes sense to have an additional field in the spec that would indicate if the tour is meant for the Admin or Dev console. Not sure if it make sense to have a tour in both ?

My question was based on the fact that we feature 3 tours on the Add page, but link out to the Guided Tours page to see "the rest".

Re: which tours are shown on the Guided Tour page, they've got to be based on privileges - I assume that someone who has no privileges to view the OperatorHub will not "see" the tours about installing Operators. @alimobrem I think this is what was discussed, right?

@jhadvig jhadvig force-pushed the workflow-guides branch 3 times, most recently from 388ec94 to d99db01 Compare June 30, 2020 16:04
@jhadvig
Copy link
Member Author

jhadvig commented Jun 30, 2020

Update the proposal based on @christianvogt and @gorkem feedback.
Was thinking about the #360 (comment). Dont think the CR can just point to Admin or Dev perspective, in fact Im not sure how the privileges should work here, cause there can be case when a Guided Tour instructs to creates several resources but user has privileges to create only subset of them.
ccíng @spadgett @serenamarie125

@christianvogt
Copy link
Contributor

@jhadvig Guided Tours feature is now renamed to 'Quick Starts'. And Guided Tours name is being reused for another feature. We may want to update the naming here now.

2. Make users understand what are the steps needed to achieve their goals.
3. Provide API for writing Guided Tours.
4. Provide new CRD format for writing the Guided Tours.
5. Have a storage for the Guided Tours.
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by "storage" here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Place where the openshift own Quick Starts will be stored(ideally a repository in openshift org)

Copy link
Member

Choose a reason for hiding this comment

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

I'd call this a repository for quick starts instead of storage.

It's worth mentioning this is only needed for out-of-the-box quick starts like those describing how to install an operator. Once the operator is installed, it can add its own quick starts.


1. Provide a mechanism to display user guides for various workflows.
2. Make users understand what are the steps needed to achieve their goals.
3. Provide API for writing Guided Tours.
Copy link
Member

Choose a reason for hiding this comment

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

Is this API something separate from the CRD?

Copy link
Member Author

Choose a reason for hiding this comment

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

no that should be the CRD

Copy link
Member

Choose a reason for hiding this comment

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

OK, I'd remove (3) since you mention the CRD just below. Otherwise it sounds like we're proposing a plugin API in addition to the CRD.

6. All Guided Tours will be listed available in a separate page that will be accessible from Help Menu.
![help-menu](https://raw.githubusercontent.com/jhadvig/images/master/help-menu.png)
7. For different areas of interest, the links for Guided Tour will need to be handles separately:
* Operators - CVS of the operator that has a Guided Tours published will contain `console.openshift.io/guided-tour: <guided-tour-name>` annotation. Upon the operator installation an "Guided Tour" link will appear next to the `Install` button.
Copy link
Member

Choose a reason for hiding this comment

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

s/CVS/CSV

GuidedTours CR for [Explore Serverless](https://marvelapp.com/236ge4ig/screen/69908905):
```
apiVersion: console.openshift.io/v1
kind: GuidedTours
Copy link
Member

Choose a reason for hiding this comment

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

Kind should be singular, GuidedTour (although I know you're working on changing the name anyway)

3. `console-operator` will import all the existing Guided Tours CRs from the `openshift/guided-tours` repository into the `/manifest` directory, so that the CVO can:
* create them if the CR doesn't exists.
* update the CRs upon the cluster update (since CVO is doing `apply`).
4. `console-operator` will check for older `GuidedTours` CR versions and remove those that doesn't match the cluster major version, which indicates that new version is not available.
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean the cluster minor version? (The y version in 4.y.z?)

We might want to support a semver version range here so one tour CR can be used for multiple OpenShift versions.


### Third party Guided Tours

Third party Guided Tours will need to be created manually on the cluster.
Copy link
Member

Choose a reason for hiding this comment

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

It's not necessarily manual. It can be created by an operator when installed.


1. In order to provide mechanism to discribe a Guided Tour, new CRD named `GuidedTours` will be created.
2. A new `openshift/guided-tours` repository will be created which will contain all supported Guided Tours.
3. `console-operator` will import all the existing Guided Tours CRs from the `openshift/guided-tours` repository into the `/manifest` directory, so that the CVO can:
Copy link
Member

Choose a reason for hiding this comment

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

Will we have a way to provide different tours for different OpenShift versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Dont think that it makes sense to have multiple QuickStart versions for a single Console version. I think the model here should be 1:1. Thats where I was aiming with the version field on the CRD

Copy link
Member

Choose a reason for hiding this comment

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

OK, are you proposing to vendor in all guided tours and filter them in the client by version?

I was thinking we would have release-* branches like in the tours repo, and the operator would just vendor from the appropriate branch. (We still may need the version property to filter tours installed by operators.)

metadata:
name: explore-serverless
spec:
version: 2.6
Copy link
Member

Choose a reason for hiding this comment

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

What is version referring to here? The version of the operator?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the confusion here. version field should correspond to a Console version for which the CR should be used.

Copy link
Member

Choose a reason for hiding this comment

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

OK, thanks. The number threw me off because it doesn't look like an OpenShift version. I would change this to something like 4.7.

@jhadvig
Copy link
Member Author

jhadvig commented Jul 17, 2020

@christianvogt @spadgett comments addressed


#### Story 4

As a operator creator I want to provide operator consumers with a guide on how to install and user the my operator.

Choose a reason for hiding this comment

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

Is there room here to mention a good content authoring experience, too?

And is it possible that tour creators might be quite different from more general "operator creators?"

* ...TBD


QuickStarts CR for [Explore Serverless](https://marvelapp.com/236ge4ig/screen/69908905):

Choose a reason for hiding this comment

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

What would the full version of this look like? The tour content is truncated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

@jhadvig Thanks! Has thought been given to externalizing the tour content specifically, so that it might be managed kind of like CCS' modularized documentation?

@jhadvig jhadvig changed the title Guided Tours for Console Quick Starts for Console Jul 18, 2020
@jhadvig
Copy link
Member Author

jhadvig commented Jul 28, 2020

@christianvogt @nemesis09 @spadgett @serenamarie125 I've updated the PR based on the discussion on the Slack and the QuickStarts implementation PR.

One thing that I think we need to sort out is whether the review, taskHelp, recapitulation should not be optional? I have an impression that they should be optional in case of a lightweight QS.
If we decide to make them optional I would suggest that review property should be an object and should contain:

  • instructions:string - would contain review instruction to validate the user's work
  • taskHelp:string - would contain suggestion with how to deal with failed task review

@rohitkrai03
Copy link

@christianvogt @nemesis09 @spadgett @serenamarie125 I've updated the PR based on the discussion on the Slack and the QuickStarts implementation PR.

One thing that I think we need to sort out is whether the review, taskHelp, recapitulation should not be optional? I have an impression that they should be optional in case of a lightweight QS.
If we decide to make them optional I would suggest that review property should be an object and should contain:

* **instructions**:string - would contain review instruction to validate the user's work

* **taskHelp**:string - would contain suggestion with how to deal with failed task review

@jhadvig I agree. I think it's better to have review as an object which has instruction and taskHelp properties.

@jhadvig
Copy link
Member Author

jhadvig commented Jul 29, 2020

@rohitkrai03 thanks for review. PR update. PTAL

cc'ing @spadgett @serenamarie125

description: introduction describes the purpose and usability of
the Quick Start. (includes markdown)
type: string
tasks:

Choose a reason for hiding this comment

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

I see the tour content library getting really big, really quickly. That means effort duplication, staleness + accuracy problems, translation pains, etc.

Could this definition support external content sources (say, OCP docs modules so that tour tasks could be single sourced?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could this definition support external content sources ...

@maxwelldb not sure what you mean by this. Do you suggest to import the fields from different CRDs ?

Choose a reason for hiding this comment

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

Potentially, yeah, or text from elsewhere.

Picture a generic operator installation tour that really only needs a specific name and maybe a configuration task to be complete.

Being able to single source content like that prevents loads of duplicated effort.

@jhadvig
Copy link
Member Author

jhadvig commented Aug 18, 2020

@christianvogt @rohitkrai03 @spadgett @serenamarie125 I've updated the PR with the spec.icon field together with description and added the spec.accessReviewResources for reviewing the access to different resource that the QuickStart is manipulating. PTAL

@christianvogt
Copy link
Contributor

lgtm

@@ -0,0 +1,173 @@
apiVersion: apiextensions.k8s.io/v1beta1
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we want to check in the CRD since it will drift from what we have in openshift/api over time. We can cover that during API review.

### Implementation Details

1. In order to provide mechanism to discribe a Quick Start, new CRD named `QuickStarts` will be created.
2. A new `openshift/quick-tours` repository will be created which will contain all supported Quick Starts. The `openshift/quick-tours` repository will have `release-*` branches.
Copy link
Member

Choose a reason for hiding this comment

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

openshift/quick-starts?

3. `console-operator` will import all the existing Quick Starts CRs from appropriate branch of the `openshift/quick-starts` repository into the `/manifest` directory, so that the CVO can:
* create them if the CR doesn't exists.
* update the CRs upon the cluster update (since CVO is doing `apply`).
4. `console-operator` will be resposible for removing older `QuickStarts` versions, based on their CRs `spec.version` field that will be in semver format. If the semver version range won't match the cluster version the `console-operator` will remove the CR.
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessarily? It seems like we can handle versioning by having release-* branches in the quick-starts repository and just vendoring the right version. I'm not sure we need a version range in the quick start resource itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

So field was mainly added, so the operator can get rid of old QuickStarts, since after the cluster update they wont be deleted from the cluster.
Also I think it cant hurt, for the QuickStart CR to know whats it's version.

Copy link
Member

Choose a reason for hiding this comment

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

Hey @jhadvig. I'm still unclear. The new quick start for the current version should just replace the old one since they have the same name. In that case, the operator doesn't need to do anything.

If we need to remove a quick start entirely, I think we might have to hard-code that special case in the operator for one release. I wouldn't expect this to happen often.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok! makes sense, will remove.


#### Quick Start icon

Each Quick Start CR can contain an icon that is specified in the `spec.icon` field. This field is either an URL of the Package Manifest that contains icon, or a base64 encoded image that are used as Quick Start's icon
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should ask quick start authors to build a URL to a package manifest icon. That's error prone and is really an internal implementation detail that could change over time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wasn't sure with it either but that's it it the outcome of the conversation I had vit @rohitkrai03 and @christianvogt.
Do you propose to cover just the base64 encoded image ?

Copy link
Member

Choose a reason for hiding this comment

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

If we want to support this, I'd suggest a specific API for referencing an operator icon by package manifest name.


### Third party Quick Starts

Third party Quick Starts will need to be created manually on the cluster, or upon operator installation.
Copy link
Member

Choose a reason for hiding this comment

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

I'd make it clear that it's the operator itself that creates these after installation.

Copy link
Member Author

Choose a reason for hiding this comment

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

How about?

### Installation

`console-operator` is responsible for installing supported Quick Starts at the end of the cluster provisioning process. Third party Quick Starts will need to be created manually on the cluster.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking something like

Suggested change
Third party Quick Starts will need to be created manually on the cluster, or upon operator installation.
Third party Quick Starts will need to be created by an operator after it's installed or manually by a cluster administrator.

@jhadvig
Copy link
Member Author

jhadvig commented Sep 9, 2020

@spadgett @christianvogt @serenamarie125 I've update the PR. Should be good to go now 👍

@jhadvig jhadvig force-pushed the workflow-guides branch 3 times, most recently from 98e94d9 to a51f739 Compare September 10, 2020 08:08
@jhadvig
Copy link
Member Author

jhadvig commented Sep 10, 2020

@spadgett removed the CRD. PTAL

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks @jhadvig

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhadvig, spadgett

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2020
@openshift-merge-robot openshift-merge-robot merged commit 8f11e0d into openshift:master Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants