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

Add new azure_manage_snapshot role #101

Merged
merged 2 commits into from
Jan 30, 2025
Merged

Conversation

p3ck
Copy link
Contributor

@p3ck p3ck commented Jan 20, 2025

New role azure_manage_snapshot will help in creating, restoring and deleting snapshots.

@p3ck p3ck force-pushed the manage_snapshot branch 3 times, most recently from 2ea87b9 to bd5e95e Compare January 20, 2025 20:33
@p3ck p3ck requested a review from mhjacks January 20, 2025 20:36
Copy link

@mhjacks mhjacks left a comment

Choose a reason for hiding this comment

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

Structure and arguments make sense. I would think it would be better to embed the community.crypto dependency in the tests if that's possible but understand if not. Otherwise LGTM

galaxy.yml Outdated Show resolved Hide resolved
@p3ck p3ck force-pushed the manage_snapshot branch 2 times, most recently from 1d498e9 to 1b075d5 Compare January 21, 2025 17:14
@mhjacks
Copy link

mhjacks commented Jan 21, 2025

Looks like we either need to exclude tests/integration/targets/azure_ops_test_azure_virtual_machine_with_public_ip/tasks/delete_all_resources.yml from lint or FQCN the tasks there. Thanks for the fixes so far, this should be the last one needed.

Copy link

@mhjacks mhjacks left a comment

Choose a reason for hiding this comment

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

Marvelous! Thanks for making all the changes. LGTM

@p3ck p3ck requested a review from anna-savina January 24, 2025 16:36
@@ -0,0 +1,3 @@
- name: Create, delete or restore Snapshot
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it's worth adding a check that mandatory variables are defined before starting?

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 thought the idea of the argument_spec.yml was supposed to enforece this.

@@ -0,0 +1,59 @@
---
Copy link
Collaborator

Choose a reason for hiding this comment

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

Other tests are run from tasks/main.yml. Not sure that the current structure can be run with the existing Jenkins job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should be launching the tests the same way as ansible upstream does.

ansible-test integration azure_ops_test_azure_manage_snapshot

Why is jenkins doing it differently?

tasks:
- name: Include default variables
ansible.builtin.include_vars:
file: ../vars/vars.yml
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use defaults/main.yml for default variables, so you don’t need an additional task.

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 cannot use defaults/main.yml because this is running as a playbook.

@@ -0,0 +1,6 @@
#!/usr/bin/env bash
Copy link
Collaborator

Choose a reason for hiding this comment

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

no runme.sh in other integration tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is normal if you need to split the tests between different hosts. The azure snapshot operations have to happen from localhost and to validate the snapshot operations have to happen on the VirtualMachine under test.

This is the accepted way to do this under ansible-test

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. Ok. I'll check tomorrow if the existing Jenkins job works with playbooks

@p3ck p3ck force-pushed the manage_snapshot branch 2 times, most recently from cbc885a to aeae51e Compare January 28, 2025 21:33
New role azure_manage_snapshot will help in creating, restoring and
deleting snapshots.
@p3ck
Copy link
Contributor Author

p3ck commented Jan 28, 2025

I don't understand the ansible-lint failure. We have the .ansible-lint file that says to ignore tests/integration dir and it did ignore it in another PR.. but now it's back.

@p3ck
Copy link
Contributor Author

p3ck commented Jan 29, 2025

I don't understand the ansible-lint failure. We have the .ansible-lint file that says to ignore tests/integration dir and it did ignore it in another PR.. but now it's back.

I think this was because the profile was wrong. I changed it to shared and it seems good

@p3ck p3ck requested a review from Yaish25491 January 30, 2025 13:51
@Yaish25491 Yaish25491 merged commit 3b2c2d1 into redhat-cop:main Jan 30, 2025
21 checks passed
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