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

ODC-5020: Add option to define (and customize) console catalog categories #763

Merged
merged 1 commit into from Dec 3, 2020

Conversation

jerolimov
Copy link
Member

@jerolimov jerolimov commented Oct 19, 2020

Issue: https://issues.redhat.com/browse/ODC-5020

Extends the existing Console CRD with new type definitions to allow the admin to override the predefined catalog categories. See enhancement proposal PR openshift/enhancements#508

Screenshot from 2020-11-06 17-04-01

@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 19, 2020
@jerolimov jerolimov closed this Oct 20, 2020
@jerolimov jerolimov reopened this Oct 20, 2020
@jerolimov jerolimov force-pushed the odc-5020 branch 3 times, most recently from 213cc1b to 09d77e2 Compare October 23, 2020 17:17
@jerolimov jerolimov changed the title [WIP] Add option to define (and customize) console catalog categories Add option to define (and customize) console catalog categories 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
@jerolimov
Copy link
Member Author

/assign @spadgett @jhadvig

Copy link
Member

@jhadvig jhadvig left a comment

Choose a reason for hiding this comment

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

Thanks for the PR ! 👍
Adding review for this CRD.

operator/v1/types_console.go Show resolved Hide resolved
operator/v1/types_console.go Outdated Show resolved Hide resolved
operator/v1/types_console.go Outdated Show resolved Hide resolved
Copy link
Member

@jhadvig jhadvig left a comment

Choose a reason for hiding this comment

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

Couple of wording suggestions.

operator/v1/types_console.go Outdated Show resolved Hide resolved
operator/v1/types_console.go Outdated Show resolved Hide resolved
operator/v1/types_console.go Outdated Show resolved Hide resolved
operator/v1/types_console.go Outdated Show resolved Hide resolved
operator/v1/types_console.go Outdated Show resolved Hide resolved
operator/v1/types_console.go Outdated Show resolved Hide resolved
@jerolimov jerolimov force-pushed the odc-5020 branch 2 times, most recently from 4e1306e to 3a98fd1 Compare December 1, 2020 12:56
@jerolimov
Copy link
Member Author

jerolimov commented Dec 1, 2020

@jhadvig I have accepted all suggestions and rebased this PR.

Tags []string `json:"tags,omitempty"`
}

// DeveloperConsoleCatalogCategory defines a top level category incl. label, filter tags
Copy link
Member

Choose a reason for hiding this comment

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

The comment here should match the API property instead of the name of the struct since this will be used for the CRD property description. I'd avoid the incl. abbreviation here since this is publicly facing doc.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jhadvig Changed type messages to:

for DeveloperConsoleCatalogCustomization:

// developerCatalog allow the cluster admin to configure developer catalog.

for DeveloperConsoleCatalogCategoryMeta and DeveloperConsoleCatalogCategory:

// category for the developer console catalog.

operator/v1/types_console.go Show resolved Hide resolved
operator/v1/types_console.go Outdated Show resolved Hide resolved
ID string `json:"id"`
// label defines a category display label.
Label string `json:"label"`
// tags in a list of strings that will match the category.
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to put more detail about how this is matched against different kinds of resources.

Copy link
Member Author

Choose a reason for hiding this comment

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

tags in a list of strings that will match the category. A selected category
show all items which has at least one overlapping tag between category and item.

OK? As you said, the exact details depends on the kind of resources

operator/v1/types_console.go Show resolved Hide resolved
operator/v1/types_console.go Show resolved Hide resolved
operator/v1/types_console.go Show resolved Hide resolved
@jerolimov
Copy link
Member Author

Updated
/woof

@openshift-ci-robot
Copy link

@jerolimov: dog image

In response to this:

Updated
/woof

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

operator/v1/types_console.go Outdated Show resolved Hide resolved
operator/v1/types_console.go Outdated Show resolved Hide resolved
operator/v1/types_console.go Outdated Show resolved Hide resolved
type DeveloperConsoleCatalogCategoryMeta struct {
// ID is an identifier used in the URL to enable deep linking in console.
// +kubebuilder:validation:Required
// +kubebuilder:validation:MinLength=1
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 also add max length validation ?

// +kubebuilder:validation:MinLength=32

or a regexp that would only allow certain chars?

// +kubebuilder:validation:Pattern=`...`

Copy link
Member

Choose a reason for hiding this comment

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

The same for Label

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, esp. for ID because we use this in URLs. Defined them now as:

	// ID is an identifier used in the URL to enable deep linking in console.
	// +kubebuilder:validation:Required
	// +kubebuilder:validation:MinLength=1
	// +kubebuilder:validation:MinLength=32
	// +kubebuilder:validation:Pattern=`^[A-Za-z0-9-_]+$`
	// +required
	ID string `json:"id"`
	// label defines a category display label.
	// +kubebuilder:validation:Required
	// +kubebuilder:validation:MinLength=1
	// +kubebuilder:validation:MinLength=64
	// +required
	Label string `json:"label"`

@jerolimov jerolimov force-pushed the odc-5020 branch 3 times, most recently from a9b27b0 to c3e53ac Compare December 3, 2020 17:44
// +kubebuilder:validation:Required
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MinLength=32
// +kubebuilder:validation:Pattern=`^[A-Za-z0-9-_]+$`
Copy link
Member

Choose a reason for hiding this comment

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

You should describe the restriction on the ID in the API doc so people know.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added them, but the pattern is also part of the CRD yaml and could not find any other doc where we add this to the description. Let me know if I should keep this or revert this.

operator/v1/types_console.go Show resolved Hide resolved
// ID is an identifier used in the URL to enable deep linking in console.
// +kubebuilder:validation:Required
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MinLength=32
Copy link
Member

Choose a reason for hiding this comment

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

MinLength 32? :) I assume you mean Max.

You should describe any restrictions in the API doc.

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, good catch, thanks. Fixed.

// label defines a category display label.
// +kubebuilder:validation:Required
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MinLength=64
Copy link
Member

Choose a reason for hiding this comment

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

MaxLength?

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, thanks. Fixed.

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 Dec 3, 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 Dec 3, 2020
@openshift-merge-robot openshift-merge-robot merged commit 3fdbb93 into openshift:master Dec 3, 2020
@jerolimov
Copy link
Member Author

jerolimov commented May 5, 2023

/retitle ODC-5020: Add option to define (and customize) console catalog categories

@openshift-ci openshift-ci bot changed the title Add option to define (and customize) console catalog categories ODC-5020: Add option to define (and customize) console catalog categories May 5, 2023
@openshift-ci openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 5, 2023
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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants