766 fixing azure rm virtualmachine top issue

[pgmgb]

SUMMARY

Fixes #766

• Removed top=1 and adjusted orderby for azure_rm_virtualmachine / get_marketplace_image_version
• Added tests for azure_rm_virtualmachine to cover latest and specific images

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

azure_rm_virtualmachine

ADDITIONAL INFORMATION

[Fred-sun]

@pgmgb I think your idea is correct, but it should not be changed in this way. My suggestions are as follows,

First: Resolve conflicts in PR.

Second: if image.version = "lastest", use top=1 and return the latest

Third: If image.version! ="latest", no top=1, returns the list to iterate over the specified image version.

[pgmgb]

@Fred-sun thank you for the review.
I'm not sure of how to implement your suggestion in a way that does not include code redundancy and more complexity.
Therefore, I would keep it that way because I only removed changes from the following pull request (#606) and did no logical/structural changes to the code.

[Fred-sun]

@pgmgb Thank you for your reply, your change is not very reasonable. I still recommend following the comments above to change.

First, please put the "import" statement back to its original position, in accordance with code design specifications.

Second, wrap your code. This is not necessary, but 160 characters per line is fine.

Third, do not delete "top=1", otherwise the first version returned is not "latest",

[pgmgb]

@Fred-sun
https://github.com/ansible-collections/azure/blob/dev/plugins/modules/azure_rm_virtualmachine.py#L2024
top=1 will add a query parameter to the API call to only return one result. The combination of top=1 and orderby='name desc' does return the latest image. But these two query parameters disable the code, which is already implemented further below. My understanding would be that removing top=1 and the sorting (desc) would fix the issue. From my point of view and understanding, no code rewrite is necessary. Please see my comments inside the code.

    def get_marketplace_image_version(self):
        try:
            versions = self.compute_client.virtual_machine_images.list(self.location,
                                                                       self.image['publisher'],
                                                                       self.image['offer'],
                                                                       self.image['sku'],
                                                                       top=1, # -> Not needed because the list would return all the time one result and disables the followed code
                                                                       orderby='name desc') # -> "desc" not needed because of the followed code (return versions[len(versions) - 1]) will not work
        except Exception as exc:
            self.fail("Error fetching image {0} {1} {2} - {3}".format(self.image['publisher'],
                                                                      self.image['offer'],
                                                                      self.image['sku'],
                                                                      str(exc)))
        if versions and len(versions) > 0:
            if self.image['version'] == 'latest':
                return versions[len(versions) - 1] # -> Will return the latest image which resists on the last position inside the array
            for version in versions:
                if version.name == self.image['version']:
                    return version

        self.fail("Error could not find image {0} {1} {2} {3}".format(self.image['publisher'],
                                                                      self.image['offer'],
                                                                      self.image['sku'],
                                                                      self.image['version']))
        return None

If my understanding is wrong or the code needs still some rewriting, I would suggest that I close this pull request and only provide the test cases to the issue #766. I'm also fine if someone else fixes the issue.

[Fred-sun]

@pgmgb Thank you very much for your contribution, other changes are ok, please put the import statement back. Thank you very much!

import-before-documentation: Import found before documentation variables. All imports must appear below DOCUMENTATION/EXAMPLES/RETURN.

[xuzhang3]

@pgmgb can you revert the format changes? Too many changes. Also please help resolve the file conflicts.

[Fred-sun]

@pgmgb Small change requests.

plugins/modules/azure_rm_virtualmachine.py:2342:0: missing-final-newline: Final newline missing

[Fred-sun]

ready_for_review

[Fred-sun]

ready_for_review

[xuzhang3]

LGTM