Skip to content

Conversation

@eroen
Copy link

@eroen eroen commented Nov 17, 2025

The UEFI state is not returned in api responses.

Please assume I don't know how ansible modules work. I haven't actually run the
test suite, but uploading snapshots works fine with and without UEFI from a
playbook.

@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

Parameter Validation

The uefi parameter is appended to resource_create_param_keys unconditionally when url is provided, but the parameter is only documented as being considered in that scenario. Ensure the API gracefully ignores uefi=false (the default) when it is not applicable, or add explicit validation to skip the parameter when it would have no effect.

self.resource_create_param_keys.append("uefi")
Duplicate Description

The new snapshot entry re-uses the description {{ vultr_resource_prefix }}_desc2, which is already assigned to the preceding snapshot. This may lead to unexpected behavior or test failures if descriptions must be unique. Consider using a distinct description such as {{ vultr_resource_prefix }}_desc2_uefi.

- description: "{{ vultr_resource_prefix }}_desc2_uefi"

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Avoid sending default uefi value

Only append "uefi" when the parameter is explicitly provided. This prevents sending
the default False value when the user omits the argument, ensuring backward
compatibility.

plugins/modules/snapshot.py [173]

-self.resource_create_param_keys.append("uefi")
+if self.module.params.get("uefi") is not None:
+    self.resource_create_param_keys.append("uefi")
Suggestion importance[1-10]: 8

__

Why: This is a critical suggestion that prevents breaking backward compatibility. The current code always sends uefi=false even when the parameter is omitted, which could change API behavior for existing users. The suggested fix ensures uefi is only sent when explicitly provided.

Medium

The UEFI state is not returned in api responses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant