Generate GX conformant credentials for VM Images

[anjastrunk]

Duplicate #69 to sign commits.

closes #67, #66

[martinmo]

I reviewed your PR and left some suggestions inline. However, I did not yet try the tool with an OpenStack cloud and plan to do this later.

As a general remark: there are a lot of commented out code lines which I think should be removed. You can always restore old code from the Git history, if necessary.

Some of the new dependencies in requirements.txt are not up-to-date anymore.

[martinmo]

One more remark: I noticed that the original issues #66 and #67 contain a lot of useful information which could be part of the docs (e.g., design/implementation decisions and information about the mapping from SCS to GX). They were helpful for me to understand the context of this PR and in my opinion it would be a pity if they are hidden in the (soon to be closed) GitHub issues.

[anjastrunk]

One more remark: I noticed that the original issues #66 and #67 contain a lot of useful information which could be part of the docs (e.g., design/implementation decisions and information about the mapping from SCS to GX). They were helpful for me to understand the context of this PR and in my opinion it would be a pity if they are hidden in the (soon to be closed) GitHub issues.

You are right. I will link both issues in documentation later on.

[anjastrunk]

@martinmo Please review my changes and approve PR.

[mbuechse]

This is some impressive reorganization.
I have mostly minor remarks, but it would be good to address them nonetheless.

[martinmo]

@anjastrunk was the merge of the other PR #67 into this PR intentional (fe1b9a7)?

[anjastrunk]

@anjastrunk was the merge of the other PR #67 into this PR intentional (fe1b9a7)?

No, it was not.

[martinmo]

I reviewed again and only found some minor things 👍

Some of the earlier suggestions have been marked as resolved but the suggestions have not been applied (these are separate steps in the GitHub UI).

In the "Docker" section of the README.md there are some examples which I don't agree with, but the section has not been changed with this PR, so I'll create a separate issue for that.

[anjastrunk]

@mbuechse @martinmo I improved code quality according to your feedback. Please review again.

[mbuechse]

Minor things. Most notable: I don't know how the EnumDefinitionImpl constructor works -- does it accept objects of the enum class in question, or does it only accept str? In the latter case, in a few instances .text should be added.

In some instances, I was wondering whether a value could be None or the empty string etc. It would be good to convey this information to the reader by making the code as explicit as possible; see my individual remarks.

[mbuechse]

even better (again, provided that disk_format can be None)

Suggested change

	if os_image.disk_format:
	return VMDiskType(
	DISK_LOOKUP.get(os_image.disk_format.lower(), VMDiskType.RAW.text)
	)
	else:
	return VMDiskType("RAW")
	if os_image.disk_format is None:
	return VMDiskType(VMDiskType.RAW.text)
	return VMDiskType(
	DISK_LOOKUP.get(os_image.disk_format.lower(), VMDiskType.RAW.text)
	)

[mbuechse]

Suggested change

	if os_image.properties and "patchlevel" in os_image.properties:
	return os_image.properties["patchlevel"]
	return os_image.properties and os_image.properties.get("patchlevel", False)

[anjastrunk]

Good hint, thanks.

[mbuechse]

Suggested change

	if os_image.properties and "internal_version" in os_image.properties:
	return os_image.properties["internal_version"]
	return os_image.properties and os_image.properties.get("internal_version", False)

[anjastrunk]

Good hint, thanks.

[mbuechse]

Do we need HypervisorType.other.text here?

[anjastrunk]

HypervisorType.other.text and HypervisorType.other are similar in this context.

[mbuechse]

@anjastrunk And one addendum to my review: it seems that your formatter (black?) is set to some very narrow line width, maybe 80 characters. That is hard to read. Maybe you can set it to 110 characters or so.

[martinmo]

As @mbuechse has suspected, every place where the constructor of an EnumDefinitionImpl subclass is used is currently broken.

For instance, VMDiskType("RAW") should be replaced with VMDiskType.RAW and so on. If this is not possible because the string is not known until runtime, you can use getattr(…): getattr(VMDiskType, type_str, VMDiskType.RAW).

[anjastrunk]

As @mbuechse has suspected, every place where the constructor of an EnumDefinitionImpl subclass is used is currently broken.

For instance, VMDiskType("RAW") should be replaced with VMDiskType.RAW and so on. If this is not possible because the string is not known until runtime, you can use getattr(…): getattr(VMDiskType, type_str, VMDiskType.RAW).

This is not correct. Constructor of every EnumDefinitionImpl is used in a right way. I used it several times throughout my tests.

@martinmo Your assumption is wrong: VMDiskType("RAW"), VMDiskType(VMDiskType.RAW) as well as VMDiskType(VMDiskType.RAW.text) construct an object of class VMDiskType, which is a subclass of EnumDefinitionImpl. EnumDefinitionImpl does not have an attribute called text. Only PermissibleValue has text as an attribute. If you want to get text of VMDiskType, you have to call VMDiskType.code.text

classDiagram
EnumDefinitionImpl <|-- VMDiskType
EnumDefinitionImpl --> PermissibleValue:code

class PermissibleValue{
    text: str
}

Loading

Calling VMDiskType.RAW, creates an object of class PermissibleValue, which is not requested, as an instance of class VMDiskType is required.

>>> from generator.common.gx_schema import VMDiskType
>>> type(VMDiskType('RAW')).__name__
'VMDiskType'
>>> type(VMDiskType(VMDiskType.RAW)).__name__
'VMDiskType'
>>> type(VMDiskType(VMDiskType.RAW.text)).__name__
'VMDiskType'
>>> type(VMDiskType.RAW).__name__
'PermissibleValue'
>>> VMDiskType('RAW').code.text
'RAW'
>>> 

[martinmo]

@martinmo Your assumption is wrong: VMDiskType("RAW"), VMDiskType(VMDiskType.RAW) as well as VMDiskType(VMDiskType.RAW.text) construct an object of class VMDiskType, which is a subclass of EnumDefinitionImpl. EnumDefinitionImpl does not have an attribute called text. Only PermissibleValue has text as an attribute. If you want to get text of VMDiskType, you have to call VMDiskType.code.text

You're right, thanks for the clarification, @anjastrunk! I am sorry and take back my comment.

For me it was confusing that the enum member is not of the enum type (like it is with Python stdlib enums) and that VMDiskType("RAW") has a misleading repr of (text='RAW'), which made me think it has a text attribute. E.g.,

>>> from generator.common.gx_schema import VMDiskType
>>> VMDiskType("RAW")
(text='RAW')

but

>>> VMDiskType("RAW").text
AttributeError: 'VMDiskType' object has no attribute 'text'

[mbuechse]

Confusing indeed.

[martinmo]

LGTM, but can you please check the mandatory attributes section README.md again. There is still a typo in the attributes, somehow the suggestion was again not applied:

• copyrigthowner (t and h switched and missing space), should be copyright owner as in the YAML.
• resourcePolicy (P should be lower case and missing space), should be resource policy as in the YAML.

(Or maybe my understanding here is wrong: I think the section talks about the keys in the YAML file.)

[anjastrunk]

@martinmo @martinmo Thanks for our tremendous feedback and very carefully review. It helped my a lot to improve code quality and documentation.

[anjastrunk]

LGTM, but can you please check the mandatory attributes section README.md again. There is still a typo in the attributes, somehow the suggestion was again not applied:

* `copyrigthowner` (t and h switched and missing space), should be `copyright owner` as in the YAML.

* `resourcePolicy` (P should be lower case and missing space), should be `resource policy` as in the YAML.

(Or maybe my understanding here is wrong: I think the section talks about the keys in the YAML file.)

Indeed. I changed it.