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

CreateNVMeSubsystem RPC #58

Merged
merged 1 commit into from
Nov 23, 2022

Conversation

harshitap26
Copy link
Contributor

@harshitap26 harshitap26 commented Nov 23, 2022

Signed-off-by: Harshita Pandey Harshita_Pandey@dell.com

Added CreateNVMeSubsystem RPC
Added CreateNVMeController RPC

@harshitap26 harshitap26 requested a review from a team as a code owner November 23, 2022 11:07
@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Merging #58 (290a2fa) into main (013f521) will increase coverage by 1.54%.
The diff coverage is 41.81%.

@@            Coverage Diff             @@
##             main      #58      +/-   ##
==========================================
+ Coverage   33.47%   35.01%   +1.54%     
==========================================
  Files           1        1              
  Lines         242      297      +55     
==========================================
+ Hits           81      104      +23     
- Misses        154      185      +31     
- Partials        7        8       +1     
Impacted Files Coverage Δ
goopicsi.go 35.01% <41.81%> (+1.54%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Harshita Pandey <Harshita_Pandey@dell.com>

Added CreateNVMeController RPC

Signed-off-by: Harshita Pandey <Harshita_Pandey@dell.com>

Added CreateNVMeControllerRPC

Signed-off-by: Harshita Pandey <Harshita_Pandey@dell.com>
Copy link
Member

@glimchb glimchb left a comment

Choose a reason for hiding this comment

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

few comments inside

response1, err := client.CreateNVMeSubsystem(ctx, &pb.CreateNVMeSubsystemRequest{
Subsystem: &pb.NVMeSubsystem{
Spec: &pb.NVMeSubsystemSpec{
Id: &pbc.ObjectKey{Value: subsystemID},
Copy link
Member

Choose a reason for hiding this comment

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

you should generate subsystemID here using, for example uuid.New() user of the library should not care about IDs... he is only aware of NQN...

if err != nil {
log.Printf("No existing NVMe Controller found with controllerID %v:", controllerID)
}
if data2 == nil {
Copy link
Member

Choose a reason for hiding this comment

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

why such a big if here ? just return immediately in case of controller found and then rest of the code is not requiring if

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This if condition is when the controller is not found corresponding to the given controllerID and thus we create a new NVMe controller. In case the controller is found, we are just logging that in line 287

Copy link
Member

Choose a reason for hiding this comment

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

what I say is just reverse the condition and return first...

if (condition) {
  // a lot of code here
}

do

if (!condition) {
    log and return
}
// a lot of code here instead

@glimchb glimchb merged commit ab31432 into opiproject:main Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants