Skip to content

Conversation

@resmo
Copy link
Collaborator

@resmo resmo commented Oct 22, 2025

Description

don't stop cleaning in case of an error in cleaning up resources

Related Issues

CI run fails

Checklist:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you linted your code locally prior to submission?
  • Have you successfully ran tests with your changes locally?

@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Silent Failures

Adding ignore_errors: true to every cleanup task may hide real infrastructure-deletion failures. Consider logging or collecting the errors so that CI can still surface them for investigation.

  ignore_errors: true
- ansible.builtin.import_tasks: cleanup_bare_metal.yml
  ignore_errors: true
- ansible.builtin.import_tasks: cleanup_ssh_key.yml
  ignore_errors: true
- ansible.builtin.import_tasks: cleanup_vpc.yml
  ignore_errors: true
- ansible.builtin.import_tasks: cleanup_vpc2.yml
  ignore_errors: true
- ansible.builtin.import_tasks: cleanup_firewall_group.yml
  ignore_errors: true
- ansible.builtin.import_tasks: cleanup_snapshot.yml
  ignore_errors: true
- ansible.builtin.import_tasks: cleanup_user.yml
  ignore_errors: true
- ansible.builtin.import_tasks: cleanup_dns_domain.yml
  ignore_errors: true
- ansible.builtin.import_tasks: cleanup_block_storage.yml
  ignore_errors: true
- ansible.builtin.import_tasks: cleanup_startup_script.yml
  ignore_errors: true
- ansible.builtin.import_tasks: cleanup_reserved_ip.yml
  ignore_errors: true
- ansible.builtin.import_tasks: cleanup_network.yml
  ignore_errors: true

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Replace ignore_errors with failed_when

Use failed_when: false instead of ignore_errors: true to suppress errors while still
marking tasks as changed when they succeed. This prevents silent failures from
masking real issues during cleanup.

tests/integration/targets/cleanup/tasks/cleanup_all.yml [7-32]

 - ansible.builtin.import_tasks: cleanup_instance.yml
-  ignore_errors: true
+  failed_when: false
 - ansible.builtin.import_tasks: cleanup_bare_metal.yml
-  ignore_errors: true
+  failed_when: false
 - ansible.builtin.import_tasks: cleanup_ssh_key.yml
-  ignore_errors: true
+  failed_when: false
 - ansible.builtin.import_tasks: cleanup_vpc.yml
-  ignore_errors: true
+  failed_when: false
 - ansible.builtin.import_tasks: cleanup_vpc2.yml
-  ignore_errors: true
+  failed_when: false
 - ansible.builtin.import_tasks: cleanup_firewall_group.yml
-  ignore_errors: true
+  failed_when: false
 - ansible.builtin.import_tasks: cleanup_snapshot.yml
-  ignore_errors: true
+  failed_when: false
 - ansible.builtin.import_tasks: cleanup_user.yml
-  ignore_errors: true
+  failed_when: false
 - ansible.builtin.import_tasks: cleanup_dns_domain.yml
-  ignore_errors: true
+  failed_when: false
 - ansible.builtin.import_tasks: cleanup_block_storage.yml
-  ignore_errors: true
+  failed_when: false
 - ansible.builtin.import_tasks: cleanup_startup_script.yml
-  ignore_errors: true
+  failed_when: false
 - ansible.builtin.import_tasks: cleanup_reserved_ip.yml
-  ignore_errors: true
+  failed_when: false
 - ansible.builtin.import_tasks: cleanup_network.yml
-  ignore_errors: true
+  failed_when: false
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that failed_when: false is better than ignore_errors: true for cleanup tasks as it suppresses errors while still allowing successful tasks to be marked as changed. This is a valid improvement for better Ansible task handling in cleanup scenarios.

Medium

@resmo resmo added the automation Trigger intergration tests label Oct 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automation Trigger intergration tests Review effort 1/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants