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.

Sign up for GitHub

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

Jump to bottom

Reconcile feature flag extensions #3987

Merged
merged 1 commit into from
Jan 22, 2020
Merged

Reconcile feature flag extensions #3987

merged 1 commit into from
Jan 22, 2020

Conversation

vojtechszocs
Copy link
Contributor

This PR aligns ModelFeatureFlag and ActionFeatureFlag extensions and their usage.

By design, every instance of ^^ represents a plugin's intent to add new Console feature flag.

To validate this invariant, the corresponding extension test (features.spec.ts) is updated to check for duplicate flag declarations.

@alecmerdler @jeff-phillips-18 I've made two major changes in the OLM plugin:

  1. Removing base flags from OLM Flags enum
 export enum Flags {
   OPERATOR_LIFECYCLE_MANAGER = 'OPERATOR_LIFECYCLE_MANAGER',
-  CAN_LIST_PACKAGE_MANIFEST = 'CAN_LIST_PACKAGE_MANIFEST',
-  CAN_LIST_OPERATOR_GROUP = 'CAN_LIST_OPERATOR_GROUP',
 }

since they are already declared in Console FLAGS enum.

  1. Removing CAN_LIST_PACKAGE_MANIFEST flag extension
-  {
-    type: 'FeatureFlag/Model',
-    properties: {
-      model: models.PackageManifestModel,
-      flag: Flags.CAN_LIST_PACKAGE_MANIFEST,
-    },
-  },

since it basically overrides the base SSAR check in public/actions/features.ts

{
  // TODO: Move into OLM plugin
  flag: FLAGS.CAN_LIST_PACKAGE_MANIFEST,
  resourceAttributes: {
    group: 'operators.coreos.com',
    resource: 'packagemanifests',
    verb: 'list',
  },
},

@bipuladh @rawagner ExtensionRegistry is modified to support the disallowed property. Dashboard related extensions should now support both disallowed and required.

Unknown flag check in public/reducers/features.ts is updated to take plugin contributed flags into account.

cc @spadgett @christianvogt

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 16, 2020
@openshift-ci-robot openshift-ci-robot added component/core Related to console core functionality component/dashboard Related to dashboard component/olm Related to OLM component/sdk Related to console-plugin-sdk component/shared Related to console-shared approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 16, 2020
@vojtechszocs vojtechszocs mentioned this pull request Jan 16, 2020
Merged
1 task
@vojtechszocs
Copy link
Contributor Author

@spadgett Adding SelfSubjectAccessReview feature flag extension would be the next follow-up step, which would allow us to address TODO: Move into OLM plugin comments in base ssarChecks.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2020
alecmerdler
alecmerdler approved these changes Jan 16, 2020
View reviewed changes
Copy link
Contributor

@alecmerdler alecmerdler 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 Jan 16, 2020
@spadgett
Copy link
Member

/hold
for #3747

@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 16, 2020
@spadgett
Copy link
Member

/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 17, 2020
bipuladh
bipuladh approved these changes Jan 17, 2020
View reviewed changes
frontend/packages/console-plugin-sdk/src/registry.ts Outdated
Comment on lines 47 to 54
.filter((e) => e.properties.required || e.properties.disallowed)
.reduce(
(requiredFlags, e) => _.uniq([...requiredFlags, ..._.castArray(e.properties.required)]),
(gatingFlags, e) =>
_.uniq([
...gatingFlags,
..._.castArray(e.properties.required),
..._.castArray(e.properties.disallowed),
]),
Copy link
Contributor

Choose a reason for hiding this comment

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

.reduce( (gatingFlags, ext) => ext.properties.required || ext.properties.disallowed ? 
    _.uniq(....)
: gatingFlags)

maybe only use reduce?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can put the filter logic inside the reduce handler.

frontend/packages/console-plugin-sdk/src/typings/features.ts
model: K8sKind;
}

export interface ActionFeatureFlag extends FeatureFlag {
/** Function used to detect the feature and set flag name/value via Redux action dispatch. */
detect: (dispatch: Dispatch) => Promise<any>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Some plugins might need to support k8sWatch which does not return a promise.

Suggested change
detect: (dispatch: Dispatch) => Promise<any>;
detect: (dispatch: Dispatch) => Promise<any> | void;

@vojtechszocs
Copy link
Contributor Author

@bipuladh @cloudbehl Regarding Bipul's #3987 (comment) - I've discussed it with @rawagner, using k8sWatch with this particular extension type can lead to problems.

On a vanilla k8s cluster or a cluster without OCS capability, CRD for OCSServiceModel is missing which causes k8sWatch for OCSServiceModel CR instance to be repeatedly failing (due to missing CRD) and causing error messages over time.

Detection of various platforms (OpenShift, Baremetal, etc.) typically uses coFetchJSON or k8sGet which are basically one-time detections. For them, using the ActionFeatureFlag extension is a good fit.

However, in case of OCS "independent" flag, driven by a specific OCSServiceModel CR instance:

k8sWatch(OCSServiceModel, { ns: 'openshift-storage', name: 'ocs-storagecluster' })

I can see two solutions here:

  1. One-time detection of OCS "independent" flag (not via websocket) - if we expect the specific OCSServiceModel CR instance to be always present on an OCS-enabled cluster.

  2. Use ModelFeatureFlag extension to detect presence of OCSServiceModel CRD, then use a (to be implemented) new extension type whose detector function executes whenever the specified Console flags change. In other words, first detect the presence of a CRD and then, once we know the CRD exists on the cluster, open a websocket and use it to drive the actual OCS "independent" flag.

@openshift-ci-robot openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 17, 2020
@vojtechszocs
Copy link
Contributor Author

Rebased and addressed #3987 (comment)

@vojtechszocs
Copy link
Contributor Author

/retest

@rawagner
Copy link
Contributor

/lgtm
/retest

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alecmerdler, bipuladh, rawagner, 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

@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.

@spadgett
Copy link
Member

/hold
the merge queue is blocked

@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 21, 2020
@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
@openshift-merge-robot openshift-merge-robot merged commit b7fbbcb into openshift:master Jan 22, 2020
@spadgett spadgett added this to the v4.4 milestone Jan 27, 2020
@vojtechszocs vojtechszocs mentioned this pull request Apr 23, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alecmerdler alecmerdler alecmerdler approved these changes

@bipuladh bipuladh bipuladh approved these changes

@andybraren andybraren Awaiting requested review from andybraren

@jeff-phillips-18 jeff-phillips-18 Awaiting requested review from jeff-phillips-18

@rawagner rawagner Awaiting requested review from rawagner

@spadgett spadgett Awaiting requested review from spadgett

Assignees

@rawagner rawagner

@alecmerdler alecmerdler

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dashboard Related to dashboard 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
Milestone
v4.4
Development

Successfully merging this pull request may close these issues.
8 participants
@vojtechszocs @spadgett @rawagner @openshift-ci-robot @openshift-bot @alecmerdler @bipuladh @openshift-merge-robot