Add a Plugins readme stub

[yaacov]

Add a plugins readme.

[yaacov]

@rhrazdil @mareklibra @vojtechszocs @suomiy @rawagner please review.

[yaacov]

/test e2e-aws-console-olm

[yaacov]

/test e2e-aws-console-olm

[yaacov]

/test e2e-aws-console-olm

[yaacov]

/test e2e-aws-console-olm

[pcbailey]

This is great, Kobi! Thanks for getting this started! =)

I just have some suggestions regarding word choice and a few grammatical and typographical fixes.

[pcbailey]

'Plugins packages' -> 'Plugin packages'

[pcbailey]

I think this would work nicely as a bulleted list. You could put the naming convention note in parentheses next to the plugin entry file item.

[vojtechszocs]

Please call them "plugin entry modules" since they are effectively ECMAscript modules.

Having package.json and OWNERS files is mandatory.

Every plugin should follow the recommended file structure:

• src directory
• integration-tests directory (if needed)
• plugin entry module at src/plugin.ts
• src/components for React components, src/models for k8s model definitions
• unit tests co-located at /path/to/unit/__tests__/unit.spec.ts

In addition, every plugin should typically provide (at least) these two extensions:

• ModelDefinition - add new k8s model definitions
• FeatureFlag/Model (in future also FeatureFlag/Action) - add new feature flag

and use the newly added feature flags (possibly together with core Console flags) to gate its extensions.

[pcbailey]

I'm assuming we're saying that unit tests are optional. I would word that like "Unit tests are encouraged, but optional. Unit tests should be placed in __tests__ directories as close as possible to the source code that they test. Integration tests should be placed in an integration-tests directory at the root level of the plugin directory."

[vojtechszocs]

Unit tests are highly encouraged. Testing is an important part of developing software.

Tests should be simple to read and consequently easy to maintain. If they're not, simplify them. Prefer smaller, focused test cases.

[pcbailey]

"Plugin directory name" -> "The plugin directory name"

[vojtechszocs]

Packages whose name starts with console- are expected to be maintained by core Console team, so they shouldn't have an OWNERS file.

Every (non-core) plugin package must have an OWNERS file. Its directory name should end with -plugin to denote its purpose.

Note: dev-console is the only exception to this rule, cc @christianvogt 😉

In addition, Console app itself is a plugin, see packages/console-app/src/plugin.tsx.

[pcbailey]

This applies to directories as well, right?

[vojtechszocs]

Yes, but I wouldn't mention it here, I think this is common knowledge (?)

[christianvogt]

I disagree with this style and it goes against other best practices. That being said this is not the right location to outline stylistic concerns. It's better managed through eslint enforcement or a separate styleguide.

[pcbailey]

"consolePlugin map, in the plugins package.json." -> "consolePlugin map in the plugin's package.json."

[pcbailey]

"Test path is relative the plugin root" -> "The test path is relative to the plugin root"

You can delete the comma after "@console/internal-integration-tests".

[pcbailey]

"OWNERS file define a list of maintainers, community plugins" -> "OWNERS file defines a list of maintainers. Community plugins".

[pcbailey]

"responsible of reviewing" -> "responsible for reviewing"

"For more in depth information see [prow]" -> "For more in-depth information, see the [prow]"

[pcbailey]

"console or community teams, plugins" -> "console or community teams. Plugins"

You can delete the comma after "OWNERS file".

[vojtechszocs]

@yaacov Posted some initial comments for your consideration.

@pcbailey Many thanks for your comments!

[vojtechszocs]

@yaacov Can you please move this file to packages/console-plugin-sdk/README.md ?

[vojtechszocs]

Many important things could be covered here:

• plugins are static meaning their code gets bundled ("baked") into Console build output
  • plugins are not loaded dynamically at runtime, which in turn simplifies the architecture (no need to sandbox plugin execution or its interaction with Console app or Bridge server)
  • console-app dependencies control the set of plugins to include in Console build, this can be overridden via CONSOLE_PLUGINS env. variable
• plugins have their code represented as Console monorepo packages
  • this increases the level of trust between Console app and its plugins
  • plugin code close to Console code avoids cross-repo PR maintenance issues
  • each plugin has an OWNERS file denoting its ownership (see here for details on reviewers and approvers)
• plugins are declarative meaning they provide a list of extensions to be interpreted
  • each extension has a unique type and properties representing its parameters (data and/or callbacks)
  • (upcoming change) each extension has flags object to support gating by Console feature flags
• Console demo plugin is used to demonstrate specific extensions
  • it should contain at least one instance of every extension type (sadly, this isn't the case yet)
  • it should be a "synthetic" (foo-bar, not-a-real-world) example acting as a starting point for further exploration

[vojtechszocs]

👍

[vojtechszocs]

Please call them "plugin entry modules" since they are effectively ECMAscript modules.

Having package.json and OWNERS files is mandatory.

Every plugin should follow the recommended file structure:

• src directory
• integration-tests directory (if needed)
• plugin entry module at src/plugin.ts
• src/components for React components, src/models for k8s model definitions
• unit tests co-located at /path/to/unit/__tests__/unit.spec.ts

In addition, every plugin should typically provide (at least) these two extensions:

• ModelDefinition - add new k8s model definitions
• FeatureFlag/Model (in future also FeatureFlag/Action) - add new feature flag

and use the newly added feature flags (possibly together with core Console flags) to gate its extensions.

[vojtechszocs]

Unit tests are highly encouraged. Testing is an important part of developing software.

Tests should be simple to read and consequently easy to maintain. If they're not, simplify them. Prefer smaller, focused test cases.

[vojtechszocs]

Plugin Entry Module 😄

[vojtechszocs]

I'd use a dotted path notation, i.e. consolePlugin.entry value.

(These aren't variables from JSON file processing point of view.)

[vojtechszocs]

For better type checking and code completion, use a type parameter that represents the union of all the extension types consumed by the plugin.

// don't do this
const plugin: Plugin<any> = [ /* stuff */ ];
// do this
const plugin: Plugin<FooExtension | BarExtension> = [ /* stuff */ ];
// or better yet, this
type ConsumedExtensions = FooExtension | BarExtension;
const plugin: Plugin<ConsumedExtensions> = [ /* stuff */ ];

[vojtechszocs]

Yup, we should.

We can refer to console-demo-plugin but it isn't complete at the moment in terms of having at least one instance of each extension type. I'm planning to write a test for that.

[vojtechszocs]

Can't we simply state that Console plugins are 1st class citizens of Console monorepo and therefore reuse the same testing (Jest, Protractor, etc.) infrastructure and conventions?

[christianvogt]

The OWNERS file should also include labels:

labels:
 - component/<plugin-name>

To add a new component labels, raise a PR here: https://github.com/openshift/release/blob/master/cluster/ci/config/prow/labels.yaml

[vojtechszocs]

Seems like the Prow label config moved to https://github.com/openshift/release/blob/master/core-services/prow/02_config/_labels.yaml

[yaacov]

Thanks ! updated link

[yaacov]

@christianvogt @vojtechszocs @pcbailey Thanks for the reviews and helpful suggestions ❗

I removed the WIP, please re-review.

[yaacov]

/test e2e-aws-console

[pcbailey]

"Plugins are static, their code gets bundled ("baked") into Console build output:"

"Plugins are static. Their code gets bundled ("baked") into the Console build output:"

[pcbailey]

"with Console app or Bridge"

"with the Console app or Bridge"

[pcbailey]

"Console-app dependencies control the set of plugins to include in Console build, this can be overridden via CONSOLE_PLUGINS env. variable."

"Console-app dependencies control the set of plugins to include in the Console build. This can be overridden using the CONSOLE_PLUGINS environment variable."

Should we provide an example of this?

[vojtechszocs]

Yes, we should.

PR #1926 documents this and provides examples for different scenarios, most notably:

• development with specific plugins
• development without any plugins

[pcbailey]

"between Console app"

"between the Console app"

[pcbailey]

"see here for details"

Is "here" supposed to be a link? If not, I'd say "see the OWNERS file section below for details" instead. Whenever I see something like "see here", I automatically assume the "here" is a link.

[vojtechszocs]

Is "here" supposed to be a link?

It was, in my original comment 😃

Whenever I see something like "see here", I automatically assume the "here" is a link.

👍

A good documentation favors specific terms, like "OWNERS file section of the Prow approve plugin", instead of loose terms, like "here" or "this link".

[pcbailey]

"The OWNERS file define a list of maintainers, community"

"The OWNERS file defines a list of maintainers. Community"

[pcbailey]

"responsible of reviewing and approving pull requests for this plugin. For more in depth information see [prow]"

"responsible for reviewing and approving pull requests for this plugin. For more in-depth information, see the [prow]"

[pcbailey]

What do the labels do/represent?

[vojtechszocs]

@pcbailey These are GitHub PR labels representing components which are touched by the given change (PR).

For example, PR that touches shared code pkg and KubeVirt plugin pkg has labels component/shared and component/kubevirt.

In general, this is a Prow CI feature that allows PRs affected by OWNERS file(s) to be labeled.

[pcbailey]

"To add a new component labels, raise a PR here: https://github.com/openshift/release/blob/master/cluster/ci/config/prow/labels.yaml"

"To add a new component label, open a PR with the new label added to https://github.com/openshift/release/blob/master/cluster/ci/config/prow/labels.yaml"

[pcbailey]

"Plugins can be owned by console or community teams, plugins maintained by the console team will not have an OWNERS file, and by convention use a directory named console-<some-name>."

"Plugins can be owned either by the Console team or community teams. Plugins maintained by the Console team will not have an OWNERS file and will use a directory named console-<some-name> by convention."

[openshift-bot]

/retest

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

[spadgett]

/hold
for #2817

[yaacov]

changing back to master :-)
for #2817 ?

[yaacov]

/test e2e-aws-console-olm

[yaacov]

/test e2e-aws

[yaacov]

/test e2e-aws

[yaacov]

/test e2e-aws

[yaacov]

/test e2e-aws

[yaacov]

/test e2e-aws

[yaacov]

@spadgett is this still do-not-merge/hold ?

[vojtechszocs]

@spadgett is this still do-not-merge/hold ?

@yaacov The master branch is now updated with 4.3 changes and open for new changes.

/hold cancel

[vojtechszocs]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pcbailey, vojtechszocs, yaacov

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:

• frontend/packages/console-plugin-sdk/OWNERS [vojtechszocs]

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

[vojtechszocs]

FYI the CONSOLE_PLUGINS env. var is described in PR #1926 - we can take examples from that.

[yaacov]

@vojtechszocs a question about CONSOLE_PLUGINS

CONSOLE_PLUGINS can only remove plugins required by console-app (?), can we add a plugin that is currently not required by console-app ( e.g. the demo-plugin or some e2e-tests-plugin ) ?

The use case I have in mind is adding a testing plugin by only using CONSOLE_PLUGINS without adding it to the console-app require var ( cc @rhrazdil )

[vojtechszocs]

CONSOLE_PLUGINS can only remove plugins required by console-app (?)

The plugins specified via the CONSOLE_PLUGINS env. variable don't have to be listed as dependencies in packages/console-app/package.json.

See resolvePluginPackages function in packages/console-plugin-sdk/src/codegen/plugin-resolver.ts - the pluginFilter parameter is not used with the CONSOLE_PLUGINS override.

can we add a plugin that is currently not required by console-app ( e.g. the demo-plugin or some e2e-tests-plugin )

Yes.

The use case I have in mind is adding a testing plugin by only using CONSOLE_PLUGINS without adding it to the console-app require var ( cc @rhrazdil )

Yes, this is a valid and supported use case 👍