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

Improve Azure Driver #1491

Closed
wants to merge 1 commit into from

Conversation

lborguetti
Copy link

@lborguetti lborguetti commented Sep 26, 2018

Guys,

This PR improves Azure Driver allowing to customize options like resource group name, location, vm size, image reference in the molecule configuration file.

Other improve is create the instance with Azure Managed Disk. The current driver does not remove the storage account created for the instance disk.

Example:

molecule/azure/molecule.yml
---
driver:
  name: azure
  location: eastus2
  resource_group_name: infra_test
platforms:
  - name: UbuntuServer16.04
    location: eastus2
    vm_size: Standard_A0
    image:
      offer: UbuntuServer
      publisher: Canonical
      sku: '16.04-LTS'
      version: latest

Let me know if this is useful.

Thanks,

…on, resource group name in the molecule configuration file
@wilmardo
Copy link
Contributor

wilmardo commented Oct 3, 2018

First of all, thanks for the PR! I think @retr0h is busy at the moment but I can see one issue already, the PR is missing an update of the Cerberus schema validation.
Take a look at #1436 for some pointers @retr0h gave me how to implement this 👍

@retr0h
Copy link
Contributor

retr0h commented Oct 4, 2018

Yes, what @wilmardo said.

@retr0h
Copy link
Contributor

retr0h commented Oct 10, 2018

I'm going to hold off from merging this. As you know Ansible is adopting Molecule, therefore I do not want to merge anything that may impact future architectural decisions, as I will no longer be leading the development of this project.

@gundalow
Copy link
Contributor

Once the repo has been moved under Ansible we can look at getting this merged.

In the short term we welcome testing and feedback on this PR.

Thanks for your understanding.

@lborguetti
Copy link
Author

lborguetti commented Oct 10, 2018

Thank you for the clarification.

@gundalow
Copy link
Contributor

@lborguetti Hi, repo move is done, so I'm reviewing the PR backlog. I see from comment about from @wilmardo that "Cerberus schema validation" may need updating, could you please take a look at that?

@lborguetti
Copy link
Author

@gundalow yes. I'll take a look.

Thanks,

Copy link
Contributor

@decentral1se decentral1se left a comment

Choose a reason for hiding this comment

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

Thanks @lborguetti, this looks solid and is a welcome change.

@@ -12,6 +16,12 @@ lint:
platforms:
{%- if cookiecutter.driver_name == 'azure' %}
- name: instance
vm_size: Standard_A0
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is also said in #1491 (comment) but I think we now need to add a schema to validate this against as in https://github.com/ansible/molecule/blob/master/molecule/model/schema_v2.py. If so, there should also be some unit tests that verify validation works as expected.

@themr0c themr0c added this to the v.2.21 milestone Jan 31, 2019
@themr0c themr0c closed this Feb 21, 2019
@themr0c
Copy link
Contributor

themr0c commented Feb 21, 2019

close/repopren to trigger travis build.

@themr0c themr0c reopened this Feb 21, 2019
@decentral1se decentral1se removed this from the v.2.21 milestone Feb 27, 2019
@ssbarnea
Copy link
Member

I would gladly support this if it passes CI and once #1840 is addressed. Until then I will mark it as blocked. The only other option to merge it is if we can find cores that can test it on their own azure accounts.

@ssbarnea ssbarnea added the blocked Blocked in some way upstream or not. label Mar 16, 2019
@decentral1se
Copy link
Contributor

Pushing out of next release as per #1491 (comment). Hope we can get moving on this soon but we're lacking some resources right now. It's on the stack!

@ssbarnea
Copy link
Member

@lborguetti can you please fix the conflicts? If you do I will make sure I test it and merge it.

@ssbarnea ssbarnea closed this Jun 14, 2019
@ssbarnea ssbarnea reopened this Jun 14, 2019
@ssbarnea ssbarnea added the azure label Aug 4, 2019
@ssbarnea
Copy link
Member

I am closing this because code changed too much since it it was created and apparently the original uploader no longer active.

If the improvements are still desired please do them in small chucks, one improvement at a time. This makes easier to review and test, less likely to have conflicts. In the end merging all improvement in small chucks would be faster than in a single one.

For example few days ago I fixed the issue of using the harcoded region.

@ssbarnea ssbarnea closed this Aug 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked in some way upstream or not. enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants