Skip to content

Conversation

@mikewurtz
Copy link
Contributor

Add export/import capabilities to/from IBM Cloud Object Storage to the image manager as well as the slcli. Unit tests updated to include tests for COS path.

@coveralls
Copy link

coveralls commented Oct 5, 2018

Coverage Status

Coverage increased (+0.02%) to 89.324% when pulling 3a6b7b3 on mikewurtz:icosImageSupport into c15db0e on softlayer:master.

Copy link
Member

@allmightyspiff allmightyspiff left a comment

Choose a reason for hiding this comment

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

Overall good change, just use the click flags on the import command and this should be good to go.

@click.option('--kp-id',
default="",
help="ID of the IBM Key Protect Instance")
@click.option('--cloud-init',
Copy link
Member

Choose a reason for hiding this comment

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

Click has an option is_flag=True, which should be used here. This way you don't need the default="" bit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change in the next commit.

help="The referenceCode of the operating system software"
" description for the imported VHD")
" description for the imported VHD, ISO, or RAW image")
@click.option('--ibm-api-key',
Copy link
Member

Choose a reason for hiding this comment

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

I'm not terribly familiar with IBM COS, but are these values obvious to someone who has used IBM COS? If not could you add some sort of info on where to get the various keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add some links to our documentation on the ibm-api-key and the wrapped-dek.

Copy link

@fode-ibm fode-ibm left a comment

Choose a reason for hiding this comment

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

Requested changes to various option default values and types.

help="ID of the root key in Key Protect")
@click.option('--wrapped-dek',
default="",
help="Wrapped Decryption Key provided by IBM KeyProtect")
Copy link

Choose a reason for hiding this comment

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

Change to, "Wrapped Data Encryption Key provided by IBM KeyProtect".

default="",
help="ID of the IBM Key Protect Instance")
@click.option('--cloud-init',
default="",
Copy link

Choose a reason for hiding this comment

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

default value should be Boolean, which should change this option type to Boolean as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default values for these flags are going to be removed in the next patch. is_flag=True, removes the need to have a default value.

default="",
help="Specifies if image is cloud init")
@click.option('--byol',
default="",
Copy link

Choose a reason for hiding this comment

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

default value should be Boolean, which should change this option type to Boolean as well.

default="",
help="Specifies if image is bring your own license")
@click.option('--is-encrypted',
default="",
Copy link

Choose a reason for hiding this comment

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

default value should be Boolean

help="ID of the IBM Key Protect Instance")
@click.option('--cloud-init',
default="",
help="Specifies if image is cloud init")
Copy link

Choose a reason for hiding this comment

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

The term, "cloud-init" should have hyphen, as it is the name of a specification. Change to, "Specifies whether image is enabled for cloud initialization using cloud-init".

@click.argument('identifier')
@click.argument('uri')
@click.option('--ibm-api-key',
default="",
Copy link

Choose a reason for hiding this comment

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

Consider changing default to None, rather than empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add on next commit.

" description for the imported VHD")
" description for the imported VHD, ISO, or RAW image")
@click.option('--ibm-api-key',
default="",
Copy link

Choose a reason for hiding this comment

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

Consider changing default to None, rather than empty string.

help="The IBM Cloud API Key with access to IBM Cloud Object "
"Storage instance.")
@click.option('--root-key-id',
default="",
Copy link

Choose a reason for hiding this comment

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

Consider changing default to None, rather than empty string.

default="",
help="Wrapped Decryption Key provided by IBM KeyProtect")
@click.option('--kp-id',
default="",
Copy link

Choose a reason for hiding this comment

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

Consider changing default to None, rather than empty string.

default="",
help="ID of the root key in Key Protect")
@click.option('--wrapped-dek',
default="",
Copy link

Choose a reason for hiding this comment

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

Consider changing default to None, rather than empty string.

@allmightyspiff allmightyspiff merged commit 17cae65 into softlayer:master Oct 9, 2018
@allmightyspiff allmightyspiff mentioned this pull request Oct 16, 2018
@mikewurtz mikewurtz deleted the icosImageSupport branch November 6, 2018 19:50
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.

4 participants