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

Allow specialized views of already registered models #1658

Closed
wants to merge 3 commits into from

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Jun 4, 2019

Allow specialized views of already registered models.

Currently we do not allow for a template to register a new view of a model already existing.

Sometimes it is useful to have a specialized view of an already registered model, for example, kubevirt use a view called "Virtual machine templates" to view openshift templates with specialized features.

Currently the new view is rejected because it is falsely flagged as duplication of the regular template view.

This PR allow for a plugin author to flag a plugin model as a specialized view, to bypass the check for duplication views.

Screenshot:
Peek 2019-06-04 15-56

@yaacov yaacov changed the title Allow specialized views of already registered models [WIP][POC]Allow specialized views of already registered models Jun 4, 2019
@openshift-ci-robot 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/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 4, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yaacov
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: benjaminapetersen

If they are not already assigned, you can assign the PR to them by writing /assign @benjaminapetersen in a comment when ready.

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

@yaacov
Copy link
Member Author

yaacov commented Jun 4, 2019

@vojtechszocs @mareklibra @rawagner please review

@alecmerdler alecmerdler self-requested a review June 4, 2019 13:54
@yaacov yaacov force-pushed the kubvirt-plugin-special-views branch from c647da1 to 3fc83ed Compare June 4, 2019 18:12
@yaacov yaacov changed the title [WIP][POC]Allow specialized views of already registered models Allow specialized views of already registered models Jun 5, 2019
@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 5, 2019
@yaacov yaacov force-pushed the kubvirt-plugin-special-views branch from a590802 to 06b4417 Compare June 5, 2019 06:05
@yaacov yaacov mentioned this pull request Jun 5, 2019
@@ -12,7 +12,7 @@ import { FLAGS } from '../../const';
const unknownKinds = new Set();

export const resourcePathFromModel = (model, name, namespace) => {
const {path, namespaced, crd} = model;
const {plural, namespaced, crd} = model;
Copy link
Member Author

Choose a reason for hiding this comment

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

@spadgett Hi, this function is building url for the console ( vs. building for the k8s API ) , shouldn't is use plural instead of path, what am I missing ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Models representing custom resources (having crd: true) will have their resourcePath based on referenceForModel function ([group]~[version]~[kind]), which serves as the key for all CRD-based models.

Models representing core k8s objects (without crd: true) will have their resourcePath based on model's path attribute only.

Model's plural attribute seems to be used for organizing models within Redux and k8s queries, see e.g. connectToPlural in public/kinds.ts.

@jelkosz
Copy link

jelkosz commented Jun 5, 2019

@vojtechszocs please have a look

@spadgett spadgett added this to the v4.2 milestone Jun 5, 2019
@vojtechszocs
Copy link
Contributor

@vojtechszocs please have a look

I've talked with @yaacov, the issue here is that we're trying to redefine an existing base k8s model (with the same [apiGroup]~[apiVersion]~[kind] key) for the sake of supporting KubeVirt's VM templates.

Redefining existing models, whether base or plugin-contributed, shouldn't be allowed. What we really want to do is have a nav link that points to a custom page that lists VM templates specifically. Using #1668 to add new route & page component seems like a better approach to me.

cc @spadgett

@vojtechszocs vojtechszocs mentioned this pull request Jun 6, 2019
3 tasks
@spadgett
Copy link
Member

spadgett commented Jun 6, 2019

Redefining existing models, whether base or plugin-contributed, shouldn't be allowed. What we really want to do is have a nav link that points to a custom page that lists VM templates specifically. Using #1668 to add new route & page component seems like a better approach to me.

cc @spadgett

+1

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 6, 2019
@yaacov
Copy link
Member Author

yaacov commented Jun 11, 2019

@spadgett @vojtechszocs @mareklibra willl use #1668 to add new route & page component for the vm template page.

@yaacov yaacov closed this Jun 11, 2019
@yaacov yaacov deleted the kubvirt-plugin-special-views branch June 11, 2019 08:17
@spadgett spadgett removed this from the v4.2 milestone Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants