Skip to content

Conversation

@bipuladh
Copy link
Contributor

@bipuladh bipuladh commented Jan 9, 2020

  • Added blocker extension point to block out the CEPH dashboard if the independent mode is present.

@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. labels Jan 9, 2020
@openshift-ci-robot openshift-ci-robot added component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/dashboard Related to dashboard component/sdk Related to console-plugin-sdk labels Jan 9, 2020
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 13, 2020
@bipuladh bipuladh force-pushed the independent-dashboard branch from 0fdfb94 to c0847fe Compare January 15, 2020 17:22
@openshift-ci-robot openshift-ci-robot added component/metal3 Related to metal3-plugin component/shared Related to console-shared labels Jan 15, 2020
@bipuladh bipuladh force-pushed the independent-dashboard branch from 7f3d8c5 to 8c3d0c9 Compare January 16, 2020 14:10
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's try to keep a clear exported API boundary between Console plugins.

  1. in ocs-independent-plugin/src/plugin.ts
export const INDEPENDENT_FLAG = 'INDEPENDENT_FLAG';
  1. add ocs-independent-plugin/src/index.ts
export { INDEPENDENT_FLAG } from './plugin';
  1. in ocs-independent-plugin/package.json
..
"private": true,
"main": "src/index.ts",
"dependencies": ..
..

and finally in ceph-storage-plugin/src/plugin.ts

import { INDEPENDENT_FLAG } from '@console/ocs-independent-plugin';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this because. ocs is dependent on ceph already. If I export this from ocs then wouldn't this cause a circular dependency issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant is the feature flag constant should live close to the Console plugin which "owns" it.

In this case, it would be ocs-independent-plugin which declares it as

{
  type: 'FeatureFlag/Model',
  properties: {
    model: OCSServiceModel,
    flag: INDEPENDENT_FLAG,
  },
},

We should avoid duplicating the same constant value across different packages.

Also, I'd suggest to modify the flag's value to CEPH_INDEPENDENT or OCS_INDEPENDENT to reflect relationship with OCS.

Copy link
Contributor

Choose a reason for hiding this comment

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

ocs is dependent on ceph already

Does it make sense to merge OCS/independent plugin code into existing Ceph plugin? These two plugins seem to be closely related.

If we want to keep them separated and avoid dependency cycle, we should move the common code into console-shared package, e.g. packages/console-shared/src/constants/flags.ts declaring shared flags.

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
_.every(requiredFlags, (f) => flags[f]) && _.every(disallowedFlags, (f) => flags[f] === false)
_.every(requiredFlags, (f) => flags[f] === true) && _.every(disallowedFlags, (f) => flags[f] === false)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest keeping flag name constants inside the plugin entry module (this file).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest keeping flag name constants inside the plugin entry module (this file).

Update: if the flag is managed via detector function, move it to corresponding features module. If it's shared between multiple packages, move it to console-shared constants.

This comment was marked as resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

The initial value should be set to undefined (feature detection in progress).

false means the feature detection is complete and feature is not available.

But I don't think we really need another action, like ActionType.AddFlag.

The simplest way to handle flags outside the base FLAGS enum is to remove unknown flag check for ActionType.SetFlag:

case ActionType.SetFlag:
  return state.set(action.payload.flag, action.payload.value);

But I think the best solution is to iterate over all extensions which deal with flags and build an array of allowed flag names, and check against that.

@bipuladh bipuladh force-pushed the independent-dashboard branch from 8c3d0c9 to 42ac2f1 Compare January 16, 2020 16:59
@vojtechszocs
Copy link
Contributor

@bipuladh For now, I'd suggest not doing any Metal3 plugin refactoring to keep this PR focused on the OCS/Independent plugin.

Also see #3987 which makes Console plugin infra changes also needed by this PR.

@bipuladh bipuladh force-pushed the independent-dashboard branch from 42ac2f1 to c45c40b Compare January 20, 2020 16:57
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. component/olm Related to OLM and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 20, 2020
@bipuladh bipuladh force-pushed the independent-dashboard branch from c45c40b to fd38b74 Compare January 20, 2020 18:14
@bipuladh bipuladh requested a review from vojtechszocs January 20, 2020 18:14
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2020
@bipuladh bipuladh changed the title [WIP] Independent Mode Dashboard Independent Mode Dashboard Jan 23, 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 23, 2020
@vojtechszocs
Copy link
Contributor

/retest

@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

@rawagner
Copy link
Contributor

/hold

build is failing due to incorrect import

ERROR in ./packages/ceph-storage-plugin/src/components/independent-dashboard-page/utilization-card/card.tsx
Module not found: Error: Can't resolve '@console/internal/components/dashboard/dashboards-page/overview-dashboard/utilization-card' in '/go/src/github.com/openshift/console/frontend/packages/ceph-storage-plugin/src/components/independent-dashboard-page/utilization-card'

@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 23, 2020
@vojtechszocs vojtechszocs force-pushed the independent-dashboard branch from 501aca1 to 7cb77df Compare January 23, 2020 17:47
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2020
@vojtechszocs
Copy link
Contributor

Fixed rebase conflict in packages/ceph-storage-plugin/src/plugin.ts and the import error.

/hold cancel

@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 23, 2020
@rawagner
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2020
@vojtechszocs vojtechszocs force-pushed the independent-dashboard branch from 7cb77df to 8384bb4 Compare January 23, 2020 18:33
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2020
@vojtechszocs
Copy link
Contributor

/retest

@rawagner
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bipuladh, rawagner, vojtechszocs

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vojtechszocs
Copy link
Contributor

/retest

@openshift-bot
Copy link
Contributor

/retest

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

3 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-merge-robot openshift-merge-robot merged commit 2e2a7b1 into openshift:master Jan 24, 2020
@spadgett spadgett added this to the v4.4 milestone Jan 27, 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. component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/dashboard Related to dashboard component/metal3 Related to metal3-plugin component/olm Related to OLM component/sdk Related to console-plugin-sdk component/shared Related to console-shared 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.

7 participants