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

Add Customize Developer Catalog Categories enhancement #508

Merged

Conversation

@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 Oct 22, 2020
@jerolimov jerolimov force-pushed the catalog-categories branch 3 times, most recently from 34f3431 to 95ebf0a Compare October 23, 2020 16:00
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.

Is this planned for 4.7? I ask because if dev console becomes a dynamic plugin in 4.8, we might want this to be part of the dev console operator config.


### Goals

- Extract the Developer Catalog categories into a CRD to allow cluster admins to customize them.
Copy link
Member

Choose a reason for hiding this comment

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

This makes it sound like we're creating a new CRD instead of just updating the console config. I'd probably phrase it just to say we're adding a configuration option for customizing the categories.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated this to

  • Add configuration options to allow cluster admins to customize catalog categories.
  • Provide a list of default values to the admin


* `id: string`, identifier used in the URL to enable deeplinking in console
* `label: string`, category display label
* `disabled: boolean`, flag to hide this category from the sidebar so that the admin is not required to remove a complete section to hide an unused category
Copy link
Member

Choose a reason for hiding this comment

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

We typically don't have this flag in our APIs. The presence of an item indicates whether something is enabled or not.

I'd assume if a category doesn't have any items it's already hidden in the UI. If this is what you mean by unused, it might be taken care of already.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed this disabled flag. The overall proposal changed a little bit to simplify things.

We can keep the existing default categories in the dev console code and will use them as fallback if there is no customization available. This makes upgrades / downgrades easier, as well we don't need to keep disabled categories in the CRD.

Christian comes with the idea of providing a code snippet to restore the current defaults when editing the Console CRD. Wdyt?


* `subcategories:` a list of Categories

If a category has subcategories the dev console will show all tags from a selected category and all subcategories.
Copy link
Member

Choose a reason for hiding this comment

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

If an item has a tag from the top-level categories, but doesn't match any tags from the subcategories, what is the behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is that a (top level) category shows all items which are shown when you go through all subcategories.

For example:

├── Languages
│   ├── Java (tags: java)
│   └── Go (tags: go)

If you select languages we want show both languages without the need to add both tags also to "Languages". Wdyt?

spec:
customization:
brand: online
devcatalog:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
devcatalog:
developerCatalog:

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed. Did you think we should keep developerCatalog.categories?
Or maybe more structured developer.catalog.categories?
Or in a case when thinking about one catalog for both sides maybe only catalog.categories?

But changed this to your recommendation for now. Thanks


### Upgrade / Downgrade Strategy

When updating to 4.7 we need to ensure that the default types was automatically added to the existing Console CR.
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 probably simpler to just hard code the defaults in the console code and use those if the stanza isn't present in the config CR. This is better as well if we add new default categories later, which would be hard if we add the defaults to the config CR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, your rigtht. I changed the proposal to keep the hard coded default values in console and "just" allow the admin to override this default so that we use the CR or static version. No merging. No migrating.

customization:
brand: online
devcatalog:
categories:
Copy link
Member

Choose a reason for hiding this comment

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

Does this completely replace the categories or just add new categories?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the document that we use the hard coded version OR the customization version. Ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

This will completely replace the dev catalog categories.


## Drawbacks

- A bigger `Console` CR must fetches by the console before the Developer Catalog is opened.
Copy link
Member

Choose a reason for hiding this comment

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

We wouldn't be fetching the CR in the frontend code. The operator will watch the config, and then wire through the values to the console backend through its config map in the openshift-console namespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks 👍 Changed the document to make it clear that the frontend reads the data from the ConfigMap.

@jerolimov jerolimov changed the title [WIP] Add Customize Developer Catalog Categories enhancement Add Customize Developer Catalog Categories enhancement Nov 6, 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 Nov 6, 2020
customization:
brand: online
devcatalog:
categories:
Copy link
Contributor

Choose a reason for hiding this comment

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

This will completely replace the dev catalog categories.

enhancements/console/catalog-categories.md Outdated Show resolved Hide resolved
enhancements/console/catalog-categories.md Outdated Show resolved Hide resolved
enhancements/console/catalog-categories.md Outdated Show resolved Hide resolved
enhancements/console/catalog-categories.md Outdated Show resolved Hide resolved
enhancements/console/catalog-categories.md Outdated Show resolved Hide resolved
enhancements/console/catalog-categories.md Outdated Show resolved Hide resolved
enhancements/console/catalog-categories.md Outdated Show resolved Hide resolved
enhancements/console/catalog-categories.md Outdated Show resolved Hide resolved
enhancements/console/catalog-categories.md Outdated Show resolved Hide resolved
@jerolimov
Copy link
Member Author

Wow, thanks @christianvogt. 👍 Applied all changes from you and marked your changes as resolved for the next round. 😏

@jerolimov
Copy link
Member Author

jerolimov commented Nov 13, 2020

@spadgett @christianvogt Dropped the section "Fetching data from ConfigMap" completely because it suggests a wrong approach.

Fetching data from ConfigMap

An update to the Console CR will update the console deployment and update a corresponding ConfigMap.

The console will read this customization from the ConfigMap "console-config" (in namespace "openshift-console").

If there is any categories defined in the ConfigMap this will override the predefined (static) list of categories. The two lists are not merged.

Anything else to-do here?

@jerolimov
Copy link
Member Author

/assign @spadgett @christianvogt @rohitkrai03

@christianvogt
Copy link
Contributor

lgtm

@spadgett could you please have a look

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

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerolimov, 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 Nov 13, 2020
@openshift-merge-robot openshift-merge-robot merged commit 0ae0bb4 into openshift:master Nov 13, 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.

None yet

6 participants