Rework unsupported security types

[JAVGan]

The previous code would always let the security_type to be any configured value in the plan as long as it was marked as a X86Gen2.

However, recently Azure introduced support for trusted boot for armGen2 which requires setting the same value for the security type as x86Gen2. With this change, we only need to ignore the values for x86Gen1 now, as it doesn't support secure boot.

This PR changes the behavior of update_skus (and _build_skus) to ONLY IGNORE the existing value for security type for x86Gen1 by default and allow the ignore list to be received as parameter, so we can properly set it without code changes in the future.

[sourcery-ai]

Reviewer's Guide

This PR introduces an environment-variable–backed configuration for unsupported security-type architectures in the Azure utils, refactors existing security‐type logic to consume the new accessor, and adds comprehensive unit tests for that accessor.

Sequence diagram for get_safe_security_type logic update

sequenceDiagram
    participant "Caller"
    participant Utils
    "Caller"->>Utils: get_safe_security_type(image_type)
    Utils->>Utils: get_unsupported_security_type_arches()
    Utils->>Utils: Check if image_type in unsupported arches
    alt image_type unsupported
        Utils-->>"Caller": return None
    else image_type supported
        Utils-->>"Caller": return security_type
    end

Loading

Class diagram for UNSUPPORTED_SECURITY_TYPE_ARCHES and get_unsupported_security_type_arches changes

classDiagram
    class AzurePublishingMetadata {
        <<inherits PublishingMetadata>>
    }
    class Utils {
        +UNSUPPORTED_SECURITY_TYPE_ARCHES: str
        +get_unsupported_security_type_arches() List[str]
    }
    Utils : UNSUPPORTED_SECURITY_TYPE_ARCHES
    Utils : get_unsupported_security_type_arches()
    AzurePublishingMetadata --|> PublishingMetadata

Loading

File-Level Changes

Change	Details	Files
Introduce environment-driven unsupported-arches configuration and accessor	Define UNSUPPORTED_SECURITY_TYPE_ARCHES using os.environ.get with a default Add get_unsupported_security_type_arches() to split the comma-separated value Refactor get_safe_security_type to use the new accessor and update its comment Update _get_security_type doc comment to reflect configurable arches	cloudpub/ms_azure/utils.py
Add unit tests for get_unsupported_security_type_arches	Import get_unsupported_security_type_arches in the test module Add tests covering default, two-values, three-values, and empty-string scenarios via mock.patch	tests/ms_azure/test_utils.py

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey there - I've reviewed your changes - here's some feedback:

• The split on an empty UNSUPPORTED_SECURITY_TYPE_ARCHES yields [""], which is likely unintended—consider filtering out empty strings so it returns an empty list instead.
• The new get_safe_security_type check uses if image_type not in get_unsupported_security_type_arches() which seems to invert the original logic (only x64Gen2 should get a security type); please verify that this condition matches the intended behavior.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The split on an empty UNSUPPORTED_SECURITY_TYPE_ARCHES yields [""], which is likely unintended—consider filtering out empty strings so it returns an empty list instead.
- The new get_safe_security_type check uses `if image_type not in get_unsupported_security_type_arches()` which seems to invert the original logic (only x64Gen2 should get a security type); please verify that this condition matches the intended behavior.

## Individual Comments

### Comment 1
<location> `cloudpub/ms_azure/utils.py:317-319` </location>
<code_context>
     return True


+def get_unsupported_security_type_arches() -> List[str]:
+    """Return the list of arches that don't support security type."""
+    return UNSUPPORTED_SECURITY_TYPE_ARCHES.split(",")
+
+
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Whitespace in environment variable values may cause subtle bugs.

Strip whitespace from each item after splitting to ensure consistent list values and prevent comparison issues.

```suggestion
def get_unsupported_security_type_arches() -> List[str]:
    """Return the list of arches that don't support security type."""
    return [arch.strip() for arch in UNSUPPORTED_SECURITY_TYPE_ARCHES.split(",")]
```
</issue_to_address>

### Comment 2
<location> `cloudpub/ms_azure/utils.py:335-336` </location>
<code_context>
     def get_safe_security_type(image_type):
-        # Arches which aren't x86Gen2 (like ARM64) doesn't work well with security type
-        if image_type != "x64Gen2":
+        # Some arches (like x86 Gen1) doesn't support security type, so we need to skip them.
+        if image_type not in get_unsupported_security_type_arches():
             return None
         return security_type
</code_context>

<issue_to_address>
**issue (bug_risk):** Logic appears inverted for skipping unsupported arches.

The current logic skips supported arches instead of unsupported ones. Update the condition to 'if image_type in get_unsupported_security_type_arches()' so only unsupported arches are skipped.
</issue_to_address>

### Comment 3
<location> `tests/ms_azure/test_utils.py:586-587` </location>
<code_context>

         assert res == disk_version_arm64_obj
+
+    @mock.patch("cloudpub.ms_azure.utils.UNSUPPORTED_SECURITY_TYPE_ARCHES", "x64Gen1")
+    def test_get_unsupported_security_type_arches_default(self) -> None:
+        """Test that the function returns the default value when env var is not set."""
+        res = get_unsupported_security_type_arches()
</code_context>

<issue_to_address>
**suggestion (testing):** Consider adding a test for whitespace handling in UNSUPPORTED_SECURITY_TYPE_ARCHES.

Please add a test case where the environment variable includes extra whitespace to ensure the function properly strips it from the values.
</issue_to_address>

### Comment 4
<location> `tests/ms_azure/test_utils.py:604-605` </location>
<code_context>
+        res = get_unsupported_security_type_arches()
+        assert res == ["x64Gen1", "x64Gen2", "arm64Gen1"]
+
+    @mock.patch("cloudpub.ms_azure.utils.UNSUPPORTED_SECURITY_TYPE_ARCHES", "")
+    def test_get_unsupported_security_type_arches_empty(self) -> None:
+        """Test that the function returns empty list when env var is empty."""
+        res = get_unsupported_security_type_arches()
</code_context>

<issue_to_address>
**question (testing):** The test for empty string returns [""], which may not be the intended behavior.

Should get_unsupported_security_type_arches() return an empty list instead of [""] when the environment variable is empty? If so, please update both the function and its test.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[JAVGan]

@lslebodn PTAL again.

Since this is a exclusion method and we only have x86Gen1 which doesn't support the security type I'm confident it will solve our issues.

I know that we will no longer plan to mix up arches in a single plan but this might be useful beside being part of code correctness. It also offers more control to us if we need to bypass a certain type of arch

[lslebodn]

Since this is a exclusion method

I do not worry much about exclusion. I am just curious how much configurable exclusion would be useful.

We should build skus based on matrix {supported Image types in plan} x {supported Security types in plan}
At the time of writting the comment, the full matrix would be of size 3x3 which gives 9 different values.
So far we can provide some defaults for sub-set.

IMHO we should either fix defaults based on expected plans settings.
Or we should provide more powerful/flexible way of override. (not just to None)

[JAVGan]

In the current code it's always whatever it's set there OR None for unsupported types. I wonder which case would we need to change things, since this is mainly controlled by the offer admins (CloudOPS) and doesn't support changing once active. With that, implementing something more flexible seems like overengineering IMHO but.. What if we start with this and later on implement that? Like as a separate PR/story?

[JAVGan]

Closing as it will be done differently, with a more flexible way of controlling the security type for new SKUs