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

HPE Nimble storage driver #740

Merged
merged 23 commits into from
May 20, 2019
Merged

Conversation

fideltak
Copy link
Contributor

What this PR does / why we need it:
Adding HPE nimble storage driver.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
N/A

Special notes for your reviewer:
This is initial req for nimble driver.

Release note:

This driver has below functions for now.
- Create volumes.
- Delete volumes.
- Create snapshots.
- Delete snapshots.
- Extend volumes.
- Attach volumes.

@xing-yang
Copy link
Collaborator

Thanks @fideltak for submitting the HPE Nimble driver! We'll take a look.

Copy link
Collaborator

@wisererik wisererik left a comment

Choose a reason for hiding this comment

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

Hi @fideltak, thanks you for developing Nimble driver. there are some suggestions for your code:

  1. Use goimport tool format go code, some go ides will help you format code, such as golite, goland.
  2. Un-used code should be removed.
  3. For error log message, please use lowercase.
  4. Add license description in the header of *.go file, like the following:
// Copyright 2019 The OpenSDS Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

contrib/drivers/hpe/nimble/client.go Show resolved Hide resolved
contrib/drivers/hpe/nimble/client.go Show resolved Hide resolved
contrib/drivers/hpe/nimble/model.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@wisererik wisererik left a comment

Choose a reason for hiding this comment

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

  1. it seems that missing initialisation and clean part in contrib/drivers/drivers.go->Init and Clean
  2. please fix CI error

examples/driver/hpe_nimble.yaml Show resolved Hide resolved
@leonwanghui
Copy link
Collaborator

@fideltak Please resolve the ci conflict error.

@coveralls
Copy link

coveralls commented May 16, 2019

Coverage Status

Coverage decreased (-0.008%) to 28.363% when pulling 1b7f4df on fideltak:nimble into 7c9ba74 on opensds:development.

contrib/drivers/hpe/nimble/nimble.go Show resolved Hide resolved
contrib/drivers/hpe/nimble/nimble.go Outdated Show resolved Hide resolved
contrib/drivers/hpe/nimble/nimble.go Show resolved Hide resolved
contrib/drivers/hpe/nimble/nimble.go Outdated Show resolved Hide resolved
contrib/drivers/hpe/nimble/nimble.go Outdated Show resolved Hide resolved
@leonwanghui leonwanghui added the feature there is a huge framework change or feature addition label May 16, 2019
Copy link
Collaborator

@leonwanghui leonwanghui left a comment

Choose a reason for hiding this comment

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

Could u use make goimports command to format the code style?

Copy link
Collaborator

@leonwanghui leonwanghui left a comment

Choose a reason for hiding this comment

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

Generally it's LGTM, please update your code

Copy link
Collaborator

@kumarashit kumarashit left a comment

Choose a reason for hiding this comment

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

Overall LGTM..
Thanks

Copy link
Collaborator

@wisererik wisererik left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@leonwanghui leonwanghui left a comment

Choose a reason for hiding this comment

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

LGTM

@wisererik wisererik merged commit e89d884 into sodafoundation:development May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature there is a huge framework change or feature addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants