search

IBM / ubiquity

Ubiquity
Apache License 2.0
90 stars 26 forks source link

Spectrumscale validation #263

Closed deeghuge closed 5 years ago

deeghuge commented 5 years ago

This change is Reviewable

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.6%) to 58.333% when pulling d51eb5bbaf5be4f31af50cda1bf1301548f4a77b on spectrumscale_validation into ae6a0a55dd776cfac7fa96c973b9f7691e2c6e2a on dev.

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 71 at r2 (raw file):

Previously, olgashtivelman wrote…
don't you want to log the error just like you do bellow with logger.ErrorRet? (same for the other places where you just return the err)

Inside validateSpectrumscaleConfig we already call logger.ErrorRet, which will log the error. Kindly let us if this addresses the comment

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 187 at r2 (raw file):

Previously, olgashtivelman wrote…
just wondering if it is not safer to do this check inside the createFileSetVolume\createFilesetQuatavolume? since i am guessing you have to so this check before calling them right? (so not to forget to add it later in case some code might change)

agree. Will put this check inside createFileSetVolume\createFilesetQuatavolume functions.

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 139 at r2 (raw file):

Previously, shay-berman wrote…
suggest you to add all these error text into a error struct and just init the error struct here. So u will be able to do unit testing on these errors. and also all the error will be in the error.go file so it will be easier to wording them.

added error structs for above errors.

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 187 at r2 (raw file):

Previously, yadaven (Yadavendra Yadav) wrote…
agree. Will put this check inside createFileSetVolume\createFilesetQuatavolume functions.

moved checkIfFSMounted checks inside createFileSetVolume\createFilesetQuatavolume.

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 499 at r2 (raw file):

Previously, shay-berman wrote…
hope u have UT for them

Yes I have added UT for above cases.

yadaven commented 5 years ago

remote/mounter/spectrumscale.go, line 34 at r2 (raw file):

Previously, shay-berman wrote…
consider to add logging line here. what about unmount?

Added the logging. Unmount already had logging.

yadaven commented 5 years ago

resources/resources.go, line 190 at r2 (raw file):

Previously, shay-berman wrote…
why its here? please create error.go inside the scale directory, and put there all the scale error. don;t use the generic resources file for that please.

moved spectrum scale error to error.go file inside spectrumscale directory.

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 115 at r1 (raw file):

Previously, shay-berman wrote…
suggest you to put the "DEFAULT_FILESYSTEM_NAME" as a const, so you can use the cost here and other references you probably have. relevant for all thr 3 above.

made the changes to have DEFAULT_FILESYSTEM_NAME/REST_USER/REST_PASSWORD as const.

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 333 at r3 (raw file):

Previously, shay-berman wrote…
r u sure you want this overhead every creation of fileset? (to go back and forth to scale in order to validate it?)

yes we need this as we do not want to create a fileset on unmounted filesystem.