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

Filter out Internal APIs in Installed Operators #3877

Merged
merged 1 commit into from
Jan 23, 2020

Conversation

bipuladh
Copy link
Contributor

@bipuladh bipuladh commented Jan 7, 2020

  • Filter Internal Cards
  • Filter Internal Tabs in Installed Operators
  • Filter Create New Button Items
  • Filter row filters and items shown in the All Instances page.

Example annotation:
operators.operatorframework.io/internal-objects: '["cephblockpools.ceph.rook.io", "cephobjectstores.ceph.rook.io"]'
Screenshot from 2020-01-08 11-41-49

@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/M Denotes a PR that changes 30-99 lines, ignoring generated files. component/olm Related to OLM labels Jan 7, 2020
@bipuladh bipuladh changed the title [WIP] Filter out Internal APIs in Installed Operators Filter out Internal APIs in Installed Operators Jan 8, 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 Jan 8, 2020
@umangachapagain
Copy link
Member

Can you provide an example of CRD/CSV annotations that is used by this filter?

@umangachapagain
Copy link
Member

Did you also filter it from the Operator Install page (one where you choose channels and namespace)?

const INTERNAL = 'operators.operatorframework.io/internal-objects';
let internals: string = _.get(annotations, INTERNAL);
if (internals) {
internals = internals.replace(/'/g, '"');
Copy link
Contributor

Choose a reason for hiding this comment

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

are we doing it because annotations are written this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. you're right. and JSON doesn't work with '.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cant this be changed from BE itself or its a convention to do so?
^^ @aruniiird

Copy link
Contributor

Choose a reason for hiding this comment

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

@spadgett Is this the general convention? Why we are not using the YAML way of creating a list and then using it directly.

@bipuladh can you paste the operators.operatorframework.io/internal-objects value, so we can better understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cloudbehl I have put it in the description.

Copy link
Contributor

@aruniiird aruniiird Jan 14, 2020

Choose a reason for hiding this comment

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

Cant this be changed from BE itself or its a convention to do so?
^^ @aruniiird

@afreen23 , I have made (and submitted) the changes in the backend PR, red-hat-storage/ocs-operator#385, and it worked (without the special handling of internals string).
But isn't it a good idea (or programming practice) to have a check, thus you don't have to be dependent on BE to provide a correct string? Regardless may have to handle parsing errors.

@@ -795,24 +814,27 @@ export const ClusterServiceVersionsDetailsPage: React.FC<ClusterServiceVersionsD
props,
) => {
const instancePagesFor = (obj: ClusterServiceVersionKind) => {
const internalObjects = getInternalNames(_.get(obj, 'metadata.annotations'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const internalObjects = getInternalNames(_.get(obj, 'metadata.annotations'));
const internalObjects = getInternalObjects(_.get(obj, 'metadata.annotations'));

[key: string]: string;
};

const getInternalNames = (annotations: Annotations) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const getInternalNames = (annotations: Annotations) => {
const getInternalObjects = (annotations: Annotations) => {

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 13, 2020
const INTERNAL = 'operators.operatorframework.io/internal-objects';
let internals: string = _.get(annotations, INTERNAL);
if (internals) {
internals = internals.replace(/'/g, '"');
Copy link
Member

Choose a reason for hiding this comment

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

Wait, why is this needed? It should parse as JSON with no changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh sorry, I missed you comment. I have the same concern.

#3877 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack. I had once gotten a malformed string. Hence put it. But I will remove it.


export const getInternalObjects = (annotations: Annotation): string[] => {
const INTERNAL = 'operators.operatorframework.io/internal-objects';
let internals: string = _.get(annotations, INTERNAL);
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
let internals: string = _.get(annotations, INTERNAL);
let internals: string = _.get(annotations, ['operators.operatorframework.io/internal-objects']);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

};

export const isInternalObject = (internalObjects: string[], objectName: string): boolean =>
internalObjects.some((obj) => obj.includes(objectName));
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a substring match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack. Will make it a strict match.

csv: ClusterServiceVersionKind,
): string[] => {
const { owned } = csv.spec.customresourcedefinitions;
return owned.filter((obj) => isInternalObject(internalObjects, obj.name)).map((obj) => obj.kind);
Copy link
Member

Choose a reason for hiding this comment

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

kind is not unique since it doesn't contain the API group, so we can't use this helper to hide items by kind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack. will use referenceForProvidedAPI.

Copy link
Member

@TheRealJon TheRealJon left a comment

Choose a reason for hiding this comment

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

Just a couple of minor suggestions on the iterative methods we are using to filter and map items.

Comment on lines 286 to 287
.filter((item) => !isInternalObject(internalObjects, item.name))
.reduce((acc, crd) => ({ ...acc, [crd.name]: crd.displayName }), {}),
Copy link
Member

Choose a reason for hiding this comment

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

Reduce can be used to filter. No need to filter and then reduce. We can expand the logic in the reduce callback to exclude items that should be filtered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

) : (
props.crdDescs
.filter((crd) => !isInternalObject(internals, crd.name))
.map((desc) => (
Copy link
Member

Choose a reason for hiding this comment

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

Suggest using reduce instead of filter -> map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

(acc, crd) =>
!isInternalObject(internalObjects, crd.name)
? { ...acc, [crd.name]: crd.displayName }
: { ...acc },
Copy link
Member

Choose a reason for hiding this comment

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

No reason to make a copy here

Suggested change
: { ...acc },
: acc,

@@ -246,3 +246,7 @@ export type OperatorGroupKind = {
lastUpdated: string;
};
} & K8sResourceKind;

export type Annotation = {
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 use ObjectMetadata['annotations'] or export the type here:

https://github.com/openshift/console/blob/master/frontend/public/module/k8s/index.ts#L39

(acc, obj) =>
isInternalObject(internalObjects, obj.name)
? [referenceForProvidedAPI(obj), ...acc]
: [...acc],
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
: [...acc],
: acc,

try {
return JSON.parse(internals);
} catch {
return [];
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 add a console.error for debugging


export const getInternalObjects = (annotations: Annotation): string[] => {
const internals: string = _.get(annotations, 'operators.operatorframework.io/internal-objects');
if (internals) {
Copy link
Member

Choose a reason for hiding this comment

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

Prefer early returns, which are easier to reason aboue.

if (!internals) {
  return [];
}

// parse json....

internalObjects.some((obj) => obj === objectName);

export const internalAPIReferences = (
internalObjects: string[],
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 just parse the internal objects here instead of taking it in as a param. That makes the API simpler.

export const isInternalObject = (internalObjects: string[], objectName: string): boolean =>
internalObjects.some((obj) => obj === objectName);

export const internalAPIReferences = (
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
export const internalAPIReferences = (
export const getInternalAPIReferences = (

@bipuladh bipuladh force-pushed the olm-internal-test branch 2 times, most recently from 358e1a0 to 50c8811 Compare January 14, 2020 14:14
internalObjects.some((obj) => obj === objectName);

export const getInternalAPIReferences = (
annotations: ObjectMetadata['annotations'],
Copy link
Member

Choose a reason for hiding this comment

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

No need to pass in the annotations. They can be ready from csv

import { ClusterServiceVersionKind } from './types';
import { referenceForProvidedAPI } from './components';

export const getInternalObjects = (annotations: ObjectMetadata['annotations']): string[] => {
Copy link
Member

Choose a reason for hiding this comment

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

This is a nit, but I think it's a better abstraction if this takes in the CSV obj instead of the annotations.

@bipuladh bipuladh requested a review from spadgett January 14, 2020 15:21
@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 Jan 22, 2020
@spadgett
Copy link
Member

@spadgett
Copy link
Member

[SEVERE] https://console-openshift-console.apps.ci-op-3834lipi-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-123dbe3b7bb38cae78e3.min.js 117:71742 TypeError: t.reduce is not a function
    at i (https://console-openshift-console.apps.ci-op-3834lipi-75d12.origin-ci-int-gce.dev.openshift.com/static/operand-chunk-53cd1a5aaac54e1ba847.min.js:1:21823)
    at https://console-openshift-console.apps.ci-op-3834lipi-75d12.origin-ci-int-gce.dev.openshift.com/static/operand-chunk-53cd1a5aaac54e1ba847.min.js:1:5613
    at Li (https://console-openshift-console.apps.ci-op-3834lipi-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-123dbe3b7bb38cae78e3.min.js:118:55337)
    at Ku (https://console-openshift-console.apps.ci-op-3834lipi-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-123dbe3b7bb38cae78e3.min.js:118:98261)
    at Bu (https://console-openshift-console.apps.ci-op-3834lipi-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-123dbe3b7bb38cae78e3.min.js:118:84008)
    at Nu (https://console-openshift-console.apps.ci-op-3834lipi-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-123dbe3b7bb38cae78e3.min.js:118:81035)
    at Iu (https://console-openshift-console.apps.ci-op-3834lipi-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-123dbe3b7bb38cae78e3.min.js:118:79608)
    at https://console-openshift-console.apps.ci-op-3834lipi-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-123dbe3b7bb38cae78e3.min.js:118:41759
    at t.unstable_runWithPriority (https://console-openshift-console.apps.ci-op-3834lipi-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-123dbe3b7bb38cae78e3.min.js:126:3878)
    at so (https://console-openshift-console.apps.ci-op-3834lipi-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-123dbe3b7bb38cae78e3.min.js:118:41488)

@bipuladh bipuladh force-pushed the olm-internal-test branch 2 times, most recently from 900e9f7 to a52c456 Compare January 22, 2020 16:03
@bipuladh
Copy link
Contributor Author

/retest

@spadgett
Copy link
Member

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 22, 2020
@spadgett
Copy link
Member

Thanks for tracking the test failure down 👍

@bipuladh
Copy link
Contributor Author

/retest

internalObjects.some((obj) => obj === objectName);

export const getInternalAPIReferences = (csv: ClusterServiceVersionKind): string[] => {
const owned: CRDDescription[] = _.flatten(
Copy link
Member

@spadgett spadgett Jan 22, 2020

Choose a reason for hiding this comment

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

Isn't spec.customresourcedefinitions.owned an array? I'd expect it to be simply

const owned: CRDDescription[] = csv?.spec?.customresourcedefinitions?.owned || [];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right. I was doing this because I was taking all CRDs both owned and required. #shithappens ;)

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.

/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bipuladh, cloudbehl, jeff-phillips-18, spadgett, TheRealJon, vojtechszocs

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

@spadgett
Copy link
Member

/retest

@openshift-bot
Copy link
Contributor

/retest

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

5 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@bipuladh
Copy link
Contributor Author

/retest

@openshift-bot
Copy link
Contributor

/retest

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

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. component/olm Related to OLM 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.