Add ConsoleYAMLSample CRD type by jhadvig · Pull Request #465 · openshift/api

jhadvig · 2019-10-11T15:07:05Z

Part of https://jira.coreos.com/browse/CONSOLE-1825 story.

benjaminapetersen · 2019-10-11T15:19:25Z

+	Title string `json:"title"`
+	// description of the YAML sample.
+	Description string `json:"description"`
+	// yam is the used YAML sample.


yam -> yaml.

"yaml is the YAML sample to display."

benjaminapetersen · 2019-10-11T15:23:10Z

+	Spec              ConsoleYAMLSampleSpec `json:"spec"`
+}
+
+// ConsoleYAMLSampleSpec is the desired YAML sample configuration.


How does this know what resource to reference in the console? I'm assuming its a customyamlsample for deployments, or another resource.

Here is the proposal for target resource in the console PR: https://github.com/openshift/console/pull/2889/files#r331471061

so is that going to be added to this ConsoleYAMLSample api?

so is that going to be added to this ConsoleYAMLSample api?

Yes, we'll need that to know the kind of resource. The alternative is to try to parse the YAML sample content for all types to filter based on group/version/kind, but I'm not sure that's practical.

well, the other option, if you foresee a need to reuse yaml samples for different resources, would be to have two different CRDs:

yaml sample

yaml sample reference/link to resource type to use it for

So you can create a single (1) and then multiple (2)s.

but i don't know if that is a valid use case or not. Or even if it's valid, if it would be common enough to be worth the effort over just creating duplicate ConsoleYamlSample objects.

Not really sure what would be the use-case for reusing the CR for different resources.
If the would be one of the solutions would be as @spadgett suggested in comment to have TargetResource that would be an array

type ConsoleYAMLSampleSpec struct { TargetResource []metav1.TypeMeta `json:",targetResource"` ...

Thinking that if it wouldn't make more sense to have a multiple YAMLs for a single TargetResource ? Since we have multiple examples for some resources, eg. buildConfigs

I'm not sure what the use case is for having more than one target resource since the API group / version / kind is also in the YAML. These should almost always match.

We do have one edge case in the UI where we show Roles and Cluster Roles in the same sidebar, but I think this pretty rare. There's always the workaround of creating more than one sample.

spadgett

Thanks @jhadvig. Will need make generate when ready

cc @bparees

spadgett · 2019-10-11T15:19:42Z

+	// description of the YAML sample.
+	Description string `json:"description"`
+	// yam is the used YAML sample.
+	Yaml string `json:"yaml"`


maybe

Suggested change

Yaml string `json:"yaml"`

YAML string `json:"yaml"`

spadgett · 2019-10-11T15:20:03Z

+type ConsoleYAMLSampleSpec struct {
+	metav1.TypeMeta `json:",inline"`
+	// title of the YAML sample.
+	Title string `json:"title"`


We'll want regex patterns for each of these strings to make sure they aren't empty strings

wont it be enough to have these fields as required in the CRD ?

No, the empty string is allowed even for required fields

spadgett · 2019-10-11T15:20:33Z

+type ConsoleYAMLSample struct {
+	metav1.TypeMeta `json:",inline"`
+	// Standard object's metadata.
+	metav1.ObjectMeta `json:"metadata,omitempty"`


remove omitempty?

spadgett · 2019-10-11T15:22:50Z

+	Spec              ConsoleYAMLSampleSpec `json:"spec"`
+}
+
+// ConsoleYAMLSampleSpec is the desired YAML sample configuration.


Suggested change

// ConsoleYAMLSampleSpec is the desired YAML sample configuration.

// ConsoleYAMLSampleSpec is the desired YAML sample configuration.

// Samples will appear with their descriptions in a samples sidebar when creating a resources in the web console.

spadgett · 2019-10-11T15:24:10Z

+}
+
+// ConsoleYAMLSampleSpec is the desired YAML sample configuration.
+type ConsoleYAMLSampleSpec struct {


Looks like we're missing a way to specify the target resource apiVersion and kind

metav1.TypeMeta contains apiVersion and kind that should target specific resource

i think that field needs a more obvious name (rather than just inlining it) if that's how you're using it, to make it clear it's defining the resourcetype this sample applies to.

Definitely, already added a comment in #465 (comment)

jhadvig · 2019-10-14T14:05:12Z

@spadgett @bparees update PR with addressed comments and adding a few of my own...
PTAL

jhadvig · 2019-10-14T15:02:53Z

Also added a validation regexp that will check if the Title, Description and YAML are non-empty or just containing whitespace chars

bparees · 2019-10-14T18:23:22Z

/approve

spadgett · 2019-10-14T19:31:21Z

@jhadvig lgtm, but will need make generate

spadgett

/lgtm

openshift-bot · 2019-10-15T14:27:26Z

/retest

Please review the full test history for this PR and help us cut down flakes.

jhadvig · 2019-10-15T14:31:13Z

/retest

benjaminapetersen · 2019-10-15T14:43:50Z

+	// YAML sample is representating.
+	TargetResource metav1.TypeMeta `json:",targetResource"`
+	// title of the YAML sample.
+	// +kubebuilder:validation:Pattern=^(.|\s)*\S(.|\s)*$


So unfortunately for this to work to generate the appropriate validation, the kubebuilder tag needs to be on a type. See https://github.com/openshift/api/pull/468/files#diff-b2e8eb141280483ab2ddd2a10a65ec30R54.

Not really difficult, but a little tedious:

// +kubebuilder:validation:Pattern=^(.|\s)*\S(.|\s)*$ type ConsoleYAMLSampleDescription string

etc.

benjaminapetersen · 2019-10-15T14:44:14Z

+	Description string `json:"description"`
+	// yaml is the YAML sample to display.
+	// +kubebuilder:validation:Pattern=^(.|\s)*\S(.|\s)*$
+	YAML string `json:"yaml"`


Same, will need kubebuilder:validation pattern on a separate type.

openshift-bot · 2019-10-15T14:52:57Z

/retest

Please review the full test history for this PR and help us cut down flakes.

benjaminapetersen · 2019-10-15T15:03:31Z

I think you may need to do the following yet:

func addKnownTypes(scheme *runtime.Scheme) error {
scheme.AddKnownTypes(GroupVersion,
		&ConsoleLink{},
		&ConsoleLinkList{},
                // other types....
                &ConsoleYAMLExample
}

in console/v1/register.go

spadgett · 2019-10-15T15:25:59Z

/lgtm

openshift-ci-robot · 2019-10-15T15:26:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, jhadvig, 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

openshift-ci-robot assigned spadgett Oct 11, 2019

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

openshift-ci-robot requested review from benjaminapetersen and spadgett October 11, 2019 15:07

benjaminapetersen reviewed Oct 11, 2019

View reviewed changes

spadgett requested changes Oct 11, 2019

View reviewed changes

jhadvig force-pushed the yaml-sample branch from c3f22d3 to 9bdac3e Compare October 14, 2019 14:04

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 14, 2019

jhadvig force-pushed the yaml-sample branch from 9bdac3e to e637bc7 Compare October 14, 2019 14:58

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

spadgett mentioned this pull request Oct 14, 2019

Add add-crd-gen line to Makefile, regenerate console extension CRDs openshift/cluster-config-operator#101

Closed

jhadvig force-pushed the yaml-sample branch from e637bc7 to 525b5d4 Compare October 15, 2019 14:21

spadgett approved these changes Oct 15, 2019

View reviewed changes

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

benjaminapetersen reviewed Oct 15, 2019

View reviewed changes

jhadvig force-pushed the yaml-sample branch from 525b5d4 to 7042f3d Compare October 15, 2019 15:02

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

Add ConsoleYAMLSample CRD type

405f985

jhadvig force-pushed the yaml-sample branch from 7042f3d to 405f985 Compare October 15, 2019 15:17

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

openshift-merge-robot merged commit 538f3e8 into openshift:master Oct 15, 2019

	// ConsoleYAMLSampleSpec is the desired YAML sample configuration.
	// ConsoleYAMLSampleSpec is the desired YAML sample configuration.
	// Samples will appear with their descriptions in a samples sidebar when creating a resources in the web console.

Conversation

jhadvig commented Oct 11, 2019

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!

jhadvig Oct 14, 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!

spadgett left a comment

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!

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 commented Oct 14, 2019

Uh oh!

jhadvig commented Oct 14, 2019

Uh oh!

bparees commented Oct 14, 2019

Uh oh!

spadgett commented Oct 14, 2019

Uh oh!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-bot commented Oct 15, 2019

Uh oh!

jhadvig commented Oct 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-bot commented Oct 15, 2019

Uh oh!

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

Uh oh!

Uh oh!

spadgett commented Oct 15, 2019

Uh oh!

openshift-ci-robot commented Oct 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

jhadvig Oct 14, 2019 •

edited

Loading

benjaminapetersen commented Oct 15, 2019 •

edited

Loading