Skip to content

Initial PR for ceph plugin package #1709

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
Merged
openshift-merge-robot merged 1 commit into from
Jun 21, 2019
Merged

Initial PR for ceph plugin package #1709

openshift-merge-robot merged 1 commit into from
Jun 21, 2019

Conversation

@cloudbehl
Copy link
Contributor

@cloudbehl cloudbehl commented Jun 13, 2019 •
edited
Loading

Initial structure PR for Ceph Plugin package.

fyi: @rawagner

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 13, 2019
@cloudbehl cloudbehl changed the title Initial PR for ceph plugin Initial PR for ceph plugin package Jun 13, 2019
@cloudbehl cloudbehl force-pushed the ceph-storage-plugin branch from fc5f7d1 to 7ffebc4 Compare June 13, 2019 07:10
@spadgett
Copy link
Member

spadgett commented Jun 13, 2019

/approve
/assign @vojtechszocs

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 13, 2019
@spadgett spadgett added this to the v4.2 milestone Jun 13, 2019
@rawagner
Copy link
Contributor

rawagner commented Jun 14, 2019

/lgtm

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jun 14, 2019

@rawagner: changing LGTM is restricted to assignees, and only openshift/console repo collaborators may be assigned issues.

Details

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vojtechszocs
vojtechszocs suggested changes Jun 14, 2019
View reviewed changes
Copy link
Contributor

@vojtechszocs vojtechszocs 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 comments, looks good overall.

frontend/packages/ceph-storage-plugin/package.json Outdated
Copy link
Contributor

@vojtechszocs vojtechszocs Jun 14, 2019

Choose a reason for hiding this comment

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

Nit-pick: should be Kubernetes (not kubernetes) and without trailing space.

Copy link
Contributor Author

@cloudbehl cloudbehl Jun 18, 2019

Choose a reason for hiding this comment

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

@vojtechszocs done

frontend/packages/ceph-storage-plugin/package.json Outdated
Copy link
Contributor

@vojtechszocs vojtechszocs Jun 14, 2019

Choose a reason for hiding this comment

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

Unless you need to declare React components in your plugin's entry module, I'd use .ts instead of .tsx.

It's a good practice to put all of your React components into separate modules (i.e. under src/components) and import & use them in the plugin entry module. This helps to keep your plugin entry small and clean.

frontend/packages/ceph-storage-plugin/package.json Outdated
Copy link
Contributor

@vojtechszocs vojtechszocs Jun 14, 2019

Choose a reason for hiding this comment

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

Nit-pick: every source should have one trailing newline.

Copy link
Contributor Author

@cloudbehl cloudbehl Jun 18, 2019

Choose a reason for hiding this comment

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

@vojtechszocs done

frontend/packages/ceph-storage-plugin/src/plugin.tsx Outdated
Copy link
Contributor

@vojtechszocs vojtechszocs Jun 14, 2019

Choose a reason for hiding this comment

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

FYI, I wrote a couple of PRs to eliminate lodash vs lodash-es duality: #1708, #1724.

(No need to change anything here for now.)

Copy link
Contributor Author

@cloudbehl cloudbehl Jun 18, 2019

Choose a reason for hiding this comment

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

Ack! Will keep an eye on these PR's once merged will update them to use lodash. Thanks

@cloudbehl cloudbehl force-pushed the ceph-storage-plugin branch from 7ffebc4 to f899380 Compare June 18, 2019 06:22
@cloudbehl
Copy link
Contributor Author

cloudbehl commented Jun 18, 2019

@vojtechszocs made the required change, please have a look.

vojtechszocs
vojtechszocs suggested changes Jun 19, 2019
View reviewed changes
frontend/packages/ceph-storage-plugin/src/plugin.ts Outdated
Copy link
Contributor

@vojtechszocs vojtechszocs Jun 19, 2019

Choose a reason for hiding this comment

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

Please use lodash instead, see packages/.eslintrc.js.

@cloudbehl cloudbehl force-pushed the ceph-storage-plugin branch from f899380 to 64133a4 Compare June 19, 2019 18:00
@cloudbehl
Copy link
Contributor Author

cloudbehl commented Jun 19, 2019

@vojtechszocs Updated the PR 👍

@cloudbehl
Copy link
Contributor Author

cloudbehl commented Jun 20, 2019

@vojtechszocs @spadgett please have a look. Updated the PR.

@vojtechszocs
Copy link
Contributor

vojtechszocs commented Jun 20, 2019

Thanks @cloudbehl the PR looks good to me.

spadgett
spadgett approved these changes Jun 20, 2019
View reviewed changes
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.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 20, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jun 20, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

@spadgett
Copy link
Member

spadgett commented Jun 20, 2019

/retest

@openshift-merge-robot openshift-merge-robot merged commit 5e84fe3 into openshift:master Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spadgett spadgett spadgett approved these changes

@kyoto kyoto Awaiting requested review from kyoto

+1 more reviewer

@vojtechszocs vojtechszocs vojtechszocs approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@vojtechszocs vojtechszocs

@spadgett spadgett

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

v4.2

Development

Successfully merging this pull request may close these issues.

6 participants

@cloudbehl @spadgett @rawagner @openshift-ci-robot @vojtechszocs @openshift-merge-robot