Skip to content

feat: add Essentials SKU create support - #950

Open
Ketki Naik (ketkimnaik) wants to merge 15 commits into
Azure:test/aio-essentials-sku-previewfrom
ketkimnaik:feature/aio-essentials-sku
Open

Ketki Naik (ketkimnaik) wants to merge 15 commits into
Azure:test/aio-essentials-sku-previewfrom
ketkimnaik:feature/aio-essentials-sku

Conversation

@ketkimnaik

Copy link
Copy Markdown
Contributor

Summary

  • add create-only --sku {Essentials,Standard}
  • send explicit SKU as a top-level instance resource field while omitting it by default
  • force OPC UA to Disabled for Essentials and warn on Stable/Preview conflicts
  • add unit and integration-scenario coverage for SKU output and OPC UA behavior
  • update the IoT Operations management client default to the 2026-10-01 GA API

Test target

This PR targets test/aio-essentials-sku-preview for qualification only. Preview API/template changes are intentionally not included here and should be made on the test branch after merge, preserving the feature branch for the eventual PR to dev.

The generated deployment template was not edited manually.

Validation

  • 105 focused orchestration tests passed
  • 244 instance resource tests passed
  • flake8 passed
  • pylint 10.00/10
  • integration scenario matrix generation passed
  • git diff --check passed

Live Essentials deployment validation remains pending a SKU-capable backend and synchronized deployment template.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@ketkimnaik Ketki Naik (ketkimnaik) changed the title [Test] Add Essentials SKU create support feat: add Essentials SKU create support Sep 29, 2026
@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing full diff (head 7eea103)

Scope: read all 10 changed files plus the surrounding code they depend on — providers/orchestration/template.py (the embedded instance blueprint) at the head SHA, providers/orchestration/resources/instances.py (parse_feature_kvp_nargs / ensure_feature_key_compat), util/az_client.py + vendor/clients/iotopsmgmt/, .github/actions/build-int-test-matrix/build_matrix.py, .github/workflows/int_test.yml, and tests/helpers.py::process_additional_args. No prior review comments existed on this PR.

Blocking

  1. azext_edge/edge/providers/orchestration/targets.py (get_ops_instance_template, the new if self.sku: instance["sku"] = {"name": self.sku}) — the aioInstance resource this writes to is pinned to "apiVersion": "2026-07-01" in azext_edge/edge/providers/orchestration/template.py, and "sku" occurs 0 times in that file at head 7eea103. So the create path submits the instance PUT at 2026-07-01, which has no SKU in its contract: the deployment will either fail validation or silently drop the field. The PR body acknowledges the template is not yet synchronized, but the consequence lands inside this PR, because it is also the PR that adds the essentials-sku CI scenario:
    • .github/test-scenarios.yml adds essentials-sku with create_args: "--sku Essentials --feature opcua.mode=Stable".
    • tests/helpers.py::process_additional_args splits on --, so sku="Essentials" and feature="opcua.mode=Stable" both reach assert_aio_instance via **create_arg_dict.
    • python-init-int runs for every scenario (.github/workflows/int_test.yml, the deploy step is not gated on tox_env), so the new assert instance_show.get("sku", {}).get("name") == sku and the az iot ops list assertion execute on every run of that scenario.
      Net: merging this alone turns the essentials-sku matrix leg red. Either gate the scenario behind test: use preview API for Essentials SKU validation #951 (land them together on the test branch), or have the scenario skip the SKU assertions until the template carries a SKU-capable apiVersion.

Suggestions

  1. azext_edge/edge/util/az_client.py — DEFAULT_IOTOPS_MGMT_API_VERSION moves to 2026-10-01 in one line, which repoints every IoT Operations management call (show, list, update, delete, brokers, dataflows, connector templates, clone, migration, upgrade), not just create. Two things worth confirming before this leaves the test branch:

    • azext_edge/edge/vendor/clients/iotopsmgmt/__init__.py re-exports only v20260701, and that package's _configuration.py defaults to "2026-07-01". get_iotops_mgmt_client passes api_version through as a kwarg, so the new value is honoured on the wire but is being served by routes/shapes generated from 2026-07-01. That works only while the two are wire-compatible — is a client regeneration expected before this reaches dev?
    • A single unregistered api-version fails all az iot ops commands, not only create. Please confirm 2026-10-01 is deployed everywhere the extension targets. (I could not verify the provider's api-version list myself — read access to a subscription with Microsoft.IoTOperations was not available to me in this run.)
  2. azext_edge/edge/commands_edge.py::_apply_essentials_sku silently rewrites an explicit --feature opcua.mode=Stable|Preview into opcua.mode=Disabled and only warns. Elsewhere this command surface fails fast on contradictory input (_validate_health_check_args raises InvalidArgumentValueError; ensure_feature_key_compat raises for unsupported feature keys — see test_iot_ops_create_block_feature_config). Raising MutuallyExclusiveArgumentError for --sku Essentials + a non-Disabled opcua.mode would be more consistent, and would avoid a user believing their explicit flag was honoured. If the silent override is deliberate (the CI scenario deliberately exercises it), that is a reasonable call — but please say so in the --sku help.

  3. azext_edge/edge/params.py — the --sku help ("Billing SKU for the IoT Operations instance. The SKU cannot be changed after creation.") does not mention that Essentials forces OPC UA off. That constraint is currently only discoverable from the example in _help.py or from the runtime warning. One extra clause in the param help would make it visible in az iot ops create -h.

  4. azext_edge/tests/edge/orchestration/test_work_unit.py::test_apply_essentials_sku imports and calls the private _apply_essentials_sku directly, so nothing asserts that create_instance actually invokes it when sku == "Essentials" (the if sku == IoTOperationsSku.ESSENTIALS.value: branch) or that sku reaches InitTargets through WorkManager(**kwargs). The existing test_work_unit create tests already assert the deployed template, so an --sku Essentials case there would lock both the branch and the plumbing.

Nits

  1. azext_edge/edge/_help.py — the new Essentials example is inserted above "Create the target instance with minimum input.", so az iot ops create -h now leads with the niche SKU case. Placing it after the minimum-input and general examples reads better.

  2. _apply_essentials_sku strips only the opcua.mode key; a user-supplied opcua.settings.* entry survives alongside the forced opcua.mode=Disabled. Harmless today (COMPAT_FEAT_KEY_SET = {"opcua.mode"} in providers/orchestration/resources/instances.py rejects every other key under strict=True), but it will stop being harmless the moment that set grows.

Checked and clean (recording the negatives so they are not re-derived):

  • Case-handling of the feature filter is a non-issue: ensure_feature_key_compat matches against the literal {"opcua.mode"}, so OPCUA.mode=Stable is rejected upstream and cannot slip past feature.partition("=")[0] != "opcua.mode".
  • sku plumbing is complete for the create path: create_instance → WorkManager → InitTargets(subscription_id=..., **kwargs) (work.py line 339), and InitTargets.__init__ stores it before get_ops_instance_template uses it.
  • az iot ops clone is not broken by this: ResourceContainer._prune_resource only filters id/systemData at the resource root, so a sku block on a cloned instance survives into the generated template.
  • The new essentials-sku scenario declaring no tox_env is correct, not an omission — build_matrix.py normalises it to "", the scenario-specific pytest step is gated on tox_env != '', and python-init-int (which owns assert_aio_instance) runs unconditionally. Same shape as the existing redeploy scenario.
  • Placing sku at the ARM resource root ({"name": ...}) rather than under properties is the correct ARM SKU shape.
  • No HISTORY.rst / VERSION finding here — this repo's CONTRIBUTING.md does not require a changelog entry and HISTORY.rst on dev is still a baseline stub.

Summary: the code itself is small, well-factored and covered at unit level; the one blocking issue is a sequencing problem — the CI scenario and int assertions added here depend on a SKU-capable deployment template that only arrives in #951.

This is an automated review and may be incomplete; please verify findings before acting on them.

Keep omitted SKU absent even when regenerated templates contain their own SKU expression, and forward SKU when the template exposes a matching parameter.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing changes since 7eea103 (head eb9be90f)

Increment is one commit — eb9be90f "Handle generated SKU template fields", azext_edge/edge/providers/orchestration/targets.py +2/-0. I re-read _handle_apply_targets, the aioInstance resource in azext_edge/edge/providers/orchestration/template.py, and azext_edge/tests/edge/orchestration/test_targets_unit.py at this head.

Blocking

None new in this increment. Finding 1 from the previous review still stands unchanged: aioInstance is pinned "apiVersion": "2026-07-01" (template.py line 1550) and sku occurs 0 times in that file at eb9be90f, so the essentials-sku CI scenario's assert instance_show.get("sku", {}).get("name") == sku still depends on a template that does not carry a SKU.

Suggestions

  1. targets.py — both new lines are inert against the bundled blueprint at this head, which is worth stating explicitly since the commit message implies behaviour change:

    • "sku": self.sku in param_to_target never reaches the deployment, because _handle_apply_targets only forwards if param in built_in_template_params and ... is not None (targets.py line 212) and TEMPLATE_BLUEPRINT_INSTANCE declares no sku parameter.
    • instance.pop("sku", None) pops a key the aioInstance resource does not have.
      As forward-compat for a regenerated template this is fine and harmless (TemplateBlueprint.copy() is a deepcopy, so nothing leaks between calls). But it means the commit is untestable today and unverified against the shape it is defending against.
  2. targets.py lines 265 / 288-290 — the two halves implement two different mechanisms for the same field, and they collide once the regenerated template actually declares sku:

    • With --sku supplied, the parameter is forwarded and instance["sku"] is overwritten with the literal {"name": self.sku} — so if the generated resource expresses SKU as [parameters('sku')], the literal wins and the forwarded parameter becomes an unused declared parameter (harmless, but it makes the generated expression dead, contrary to the "template was not edited manually" intent).
    • With --sku omitted, _handle_apply_targets's is not None filter skips the parameter entirely. If the regenerated template declares sku without a default value, the deployment then fails with a missing-parameter error rather than omitting the SKU — the exact case this commit is trying to make safe.
      Recommend picking one: either drive it purely through the parameter (and give the parameter a default / nullable shape), or keep the literal injection and drop the param_to_target entry.
  3. No test locks either line. test_init_targets_instance_sku (test_targets_unit.py line 498) passes identically with and without this commit, because the blueprint has no sku to pop and no sku parameter to forward. A case that injects a blueprint carrying "sku": "[parameters('sku')]" on aioInstance (and a declared sku parameter) would actually exercise both halves — and would surface the no-default question in point 2.

Nits

None.

Checked and clean (recording the negatives so the next run does not re-derive them):

  • TemplateBlueprint.copy() is deepcopy(self.content) (template.py line 44), so instance.pop(...) cannot mutate the module-level blueprint across invocations — the obvious "shared mutable blueprint" hypothesis dies here.
  • The pop is correctly ordered before the if self.sku: assignment, so an explicit SKU is never dropped.
  • _handle_apply_targets's is not None guard means the new param_to_target entry cannot produce an "unexpected parameter" ARM validation error against the current template.
  • Repo-wide grep for sku outside vendor/ shows the consumer set is unchanged by this commit: params.py, commands_edge.py, targets.py, _help.py, and the three test modules.
  • Findings 2-7 of the previous review are untouched by this increment and are not restated here.

Summary: a small, defensive forward-compat commit that is currently a no-op; no new blockers. The substantive ask is to settle whether SKU flows through the template parameter or through the literal injection before the regenerated template lands, and to add a test that exercises the parameter-bearing blueprint.

This is an automated review and may be incomplete; please verify findings before acting on them.

Strip trailing hyphens after limiting ADR namespace names so longer scenarios such as Essentials remain valid.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing changes since eb9be90f (head 2348112)

Increment is one commit — 23481120 "Fix truncated integration namespace names", .github/actions/setup-cluster-resources/action.yml +2/-2. I read the whole composite action at this head, traced inputs.suffix back to its only caller, and enumerated the name each truncation actually produces.

Blocking

None.

Suggestions

  1. .github/actions/setup-cluster-resources/action.yml line 63 vs lines 49-50 — the sed 's/-*$//' guard was applied to one of the three truncations in this script, and the sibling that runs first has the same latent defect.

    inputs.suffix is env.CLUSTER_NAME = opt${{ github.run_number }}${{ matrix.scenario.name }} (.github/workflows/int_test.yml line 172, passed at line 246), and several scenario names in .github/test-scenarios.yml contain hyphens (essentials-sku, insecure-listener, trustbundle). Composing those against the current run number (int_test.yml is at run ~1267) gives:

    • line 63, init-adr-ns- + cut -c 1-30 → init-adr-ns-opt1267essentials- — exactly the name this commit fixes, so the diagnosis is right.
    • line 49, init-registry + cut -c 1-30 → safe at a 4-digit run number, but init-registryopt126essentials- (3-digit) and init-registryopt12670insecure- (5-digit) both land on a hyphen. The prefix is one character longer than the ADR one, which is the only reason it is not failing today; the boundary shifts every time github.run_number gains a digit.
    • line 33 (storage) is genuinely immune — tr -d '-' removes hyphens before the cut.
    • line 50, init-ns- + cut -c 1-20 produced no trailing hyphen for any scenario × run-number-length combination I tried, but it is the same construction and the same one-character-away situation.

    Since set -e is in effect and the schema registry is created before the ADR namespace, a hyphen landing on line 49 fails the job earlier and looks like a different bug. Applying the same | sed 's/-*$//' to lines 49 and 50 — or better, factoring a small trim_name() shell function used by all three — makes the fix cover the class rather than the instance.

Nits

  1. sed 's/-*$//' can in principle empty the string (all-hyphen input); not reachable here because every name has a literal alphanumeric prefix, so no guard is needed — noting it only so the next reader does not have to re-derive it.

Duplicate commit across two open PRs

#956 (same base branch test/aio-essentials-sku-preview) carries commit 10f804c7 with the same title and the same action.yml change. Whichever of #950/#956 merges second will either conflict or become a no-op on that file. Worth deciding which PR owns the fix rather than landing it twice.

Checked and clean

  • The change is confined to a CI composite action; no product code, no test, so no HISTORY.rst entry is expected in this repo (CONTRIBUTING.md does not require one and HISTORY.rst on dev is a baseline stub).
  • The ADR namespace id is consumed downstream via ADR_NAMESPACE_ID (the full ARM id), not by name, so shortening the name does not break --ns-resource-id plumbing.
  • cluster_cleanup.yml matches on resource group, not on these generated names, so a name-shape change does not orphan resources.

Still open from the earlier reviews of this PR, unchanged at this head

aioInstance is still pinned "apiVersion": "2026-07-01" in azext_edge/edge/providers/orchestration/template.py (~line 1550) with zero sku occurrences in that file, while this PR's own essentials-sku CI scenario asserts the SKU round-trips through assert_aio_instance — and python-init-int runs for every scenario because the deploy step in int_test.yml is not gated on tox_env. The eb9be90f template-plumbing lines remain no-ops against the bundled blueprint until the regenerated template declares sku.

This is an automated review and may be incomplete.

@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing changes since 2348112 (head 5223d0f)

Increment is one commit — 5223d0fc "Add licensing terms link to Essentials warning" — touching two files (+6/-1): the warning string in azext_edge/edge/commands_edge.py::_apply_essentials_sku and a matching assertion in azext_edge/tests/edge/orchestration/test_work_unit.py::test_apply_essentials_sku.

Blocking

None.

Suggestions

None for this increment. One item carried from the earlier reviews is still open at this head and is restated only so it is not lost — not a new finding:

  • azext_edge/edge/providers/orchestration/targets.py writes instance["sku"] = {"name": self.sku} (line ~290) onto the aioInstance resource, which azext_edge/edge/providers/orchestration/template.py (line ~1550) still pins at "apiVersion": "2026-07-01". The vendored GA client azext_edge/edge/vendor/clients/iotopsmgmt/ (only v20260701 is shipped) contains zero sku references, so nothing in-repo shows the GA contract accepting the property. Worth a confirmation from the RP rather than a code change here.

Nits

None.

Checked and clean

  • The URL is correct and the anchor is live. GET https://www.microsoft.com/licensing/terms/productoffering/MicrosoftAzure/MOSA returns 200 and contains <h3 class="clause-heading" id="clause-2792-h3-1">Azure IoT Operations Essentials</h3>, so the fragment lands on exactly the intended section rather than the top of a 480 KB page.
  • The clause substantiates both claims the warning makes. That section reads "Customer may (1) only deploy on a single Kubernetes node …" and, under Use Limitations, "Customer may not use OPC UA connector with Azure IoT Operations Essentials" — so "intended for single-node deployments and does not support OPC UA" is accurate, and the link is the right citation for it.
  • No lazy-formatting hazard. The first logger.warning(...) is a single literal with no % args and the URL contains no %, so the adjacent %s-style call (the OPC UA conflict warning, which does pass opcua_mode as an arg) is unaffected.
  • Test assertion targets the right element. warnings = [call.args for call in mocked_logger.warning.call_args_list], so warnings[0][0] is the format string of the first warning — the one that gained the URL. The existing len(warnings) == 1 / warnings[1][1] == conflict_mode branches are untouched and all four parametrize cases still reach the new assertion, since the licensing warning is emitted unconditionally before the OPC UA branch.
  • No stale copy of the old wording. A tree-wide grep at this head finds the licensing URL in exactly the two changed files, and Essentials in product code only at commands_edge.py (45/51), providers/orchestration/common.py:109 (IoTOperationsSku.ESSENTIALS) and the _help.py example (~1966). The --sku help in azext_edge/edge/params.py (~1270) and the _help.py example describe the SKU without repeating the limitation text, so neither needed to move with this commit.
  • Repo conventions: azure-iot-ops-cli-extension's HISTORY.rst is a stub and CONTRIBUTING.md does not require an entry, so a warning-text change needs none (this differs from azure-iot-cli-extension).

Summary: a two-line documentation improvement to an existing warning, with the assertion added alongside it. Verified externally against the licensing page; nothing to change.

This is an automated review and may be incomplete.

@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing changes since 5223d0f (head 50ea0c5f)

Increment is one commit — 50ea0c5f "Move Essentials limitations to CLI help" (3 files, +7/-14): the unconditional Essentials warning is deleted from _apply_essentials_sku and the same information is folded into the --sku help string, with test_apply_essentials_sku re-indexed accordingly. No Blocking findings.

Suggestions

  1. azext_edge/edge/commands_edge.py (_apply_essentials_sku, ~lines 38-53) — after this commit, the implicit feature mutation has no runtime signal at all. The function unconditionally strips any opcua.mode=* entry and appends opcua.mode=Disabled to instance_features, but the only surviving logger.warning is gated on opcua_mode in ("Stable", "Preview") — i.e. it fires only when the user explicitly asked for OPC UA. So the common path, az iot ops create ... --sku Essentials with no --feature, now silently rewrites the instance's feature set and prints nothing. Parameter help is rendered by az iot ops create -h, not during execution, so it does not cover this. Worth keeping a one-line notice on the mutation itself (e.g. warn when effective_features != (instance_features or []), or at minimum logger.info), rather than dropping the only in-band evidence that the command changed what the user asked for.

Nits

  1. azext_edge/edge/params.py line 1273 — "If omitted, Standard is used" is a service-side assertion that nothing in this repo corroborates. ops_init passes sku=None straight through, and azext_edge/edge/providers/orchestration/targets.py lines 288-290 do instance.pop("sku", None) then set it only if self.sku, so an omitted --sku sends no sku property at all; the vendored azext_edge/edge/vendor/clients/iotopsmgmt/ client has zero sku references, so the default lives entirely in the RP. Accurate-if-true and a useful thing to tell users — just flagging that it is unverifiable from the tree, so it needs a service-side confirmation rather than a code check.

  2. azext_edge/edge/_help.py lines 1966-1967 already carry the same prose on the Essentials example ("intended for single-node deployments and disables OPC UA"), and it is now worded differently from params.py ("does not support OPC UA"). Two hand-maintained copies of one limitation statement; worth picking one phrasing.

Carried from the 5223d0f review (not new, not re-derived)

  • targets.py (~line 290) writes instance["sku"] = {"name": self.sku} onto an aioInstance that azext_edge/edge/providers/orchestration/template.py line 1550 pins at apiVersion 2026-07-01, and the vendored iotopsmgmt client (only v20260701) still has zero sku references at this head. Unchanged by this commit.

Checked and clean

  • The test re-index is correct, not just green: the surviving call is logger.warning("OPC UA mode '%s' ... ", opcua_mode) (lazy %s), so warnings[0][0] is the format string and warnings[0][1] is opcua_mode — matching the new assert warnings[0][1] == conflict_mode / assert "will use opcua.mode=Disabled" in warnings[0][0], and assert not warnings is right for the non-conflict case.
  • Nothing else depended on the removed text: grep "single-node" over the tree at head hits only params.py and the _help.py example; clause-2792 now appears only in params.py. No int test, doc or scenario asserted the deleted warning.
  • .github/test-scenarios.yml line 47 drives the Essentials scenario with --feature opcua.mode=Stable, so it exercises the surviving conflict warning — the removal does not blind that scenario.
  • logger is still used in commands_edge.py (lines 45 and 117), so no unused-import / lint fallout from the deletion.
  • The split string literal reconstructs the licensing URL exactly (.../MicrosoftAzure/MOSA#clause-2792-h3-1), the anchor previously verified as a real element on that page; and since the URL now lives in a help string rather than a logger.warning format argument, the %-in-lazy-format hazard noted last time no longer applies.

Automated review — may be incomplete; please treat findings as input, not as a gate.

dependabot Bot and others added 8 commits October 6, 2026 12:04
… 4.38.2 (Azure#963)

chore(actions-deps): bump github/codeql-action/analyze

Bumps [github/codeql-action/analyze](https://github.com/github/codeql-action) from 4.38.1 to 4.38.2.
- [Release notes](https://github.com/github/codeql-action/releases)
- [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md)
- [Commits](github/codeql-action@1c5b675...2892aa5)

---
updated-dependencies:
- dependency-name: github/codeql-action/analyze
  dependency-version: 4.38.2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Hangyi <hangyiwang@microsoft.com>
…38.2 (Azure#962)

chore(actions-deps): bump github/codeql-action/init

Bumps [github/codeql-action/init](https://github.com/github/codeql-action) from 4.38.1 to 4.38.2.
- [Release notes](https://github.com/github/codeql-action/releases)
- [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md)
- [Commits](github/codeql-action@1c5b675...2892aa5)

---
updated-dependencies:
- dependency-name: github/codeql-action/init
  dependency-version: 4.38.2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Hangyi <hangyiwang@microsoft.com>
* Add GA preview runtime support

* Fix preview runtime compatibility handling

* Use structured runtime readiness issues

* Use supported preview management API

* Preserve diagnostic instance show access

* Fix runtime writes and help

* Refresh preview runtime and tests

* Refresh preview runtime to 22

* Address runtime profile review and support bundle fixes

Use tags-only PATCH without runtime discovery, separate Edge Registry support collection, and restore terminal-state recovery and stable force behavior. Identity PATCH changes remain pending RP confirmation.

* Add required header to Edge Registry collector

* Fix instance API and initialization

* Temporarily restore July API default

* Refresh GA and preview runtimes and simplify preview opt-in

* Fix preview CI, GA force upgrades, and broker checks

* Fix preview listener API

* Restore forced integration upgrades

* Fix unbundled integration upgrade paths

* Simplify preview upgrade error wording

* Reuse installed runtime for upgrade reconciliation

* Remove obsolete create confirmation ignore

---------

Co-authored-by: Ketki <ketkinaik@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@zzcodework

Copy link
Copy Markdown

Azure SRE Agent - automated review

Reviewing changes since 50ea0c5f (head 0ccbf4b0)

The increment is a single merge commit — 0ccbf4b0 "Merge dev into feature/aio-essentials-sku" (parents 50ea0c5f + fbe4b360, which is #948 "support GA and preview runtimes" on dev). No new Essentials commits. Because the PR targets test/aio-essentials-sku-preview, the merge inflates the PR diff to 79 files, but all of that extra surface is already-reviewed dev code (the runtime*.py subsystem, template_preview.py, the dependabot action bumps). I did not re-review it.

What I did review is the part a merge can actually break: the conflict resolutions. Replaying the merge (git merge-tree 50ea0c5f fbe4b360) shows four real conflicts — azext_edge/edge/params.py, azext_edge/edge/providers/orchestration/targets.py, azext_edge/edge/util/az_client.py, azext_edge/tests/edge/init/int/test_init_int.py — all resolved by hand (co-authored by Copilot), which is exactly where a side gets silently dropped.

Blocking

None found.

Suggestions

  1. azext_edge/edge/providers/orchestration/resources/instances.py (Instances._update, the opcua_backfill_requested block, ~lines 390-445) — the SKU is enforced only at create time. _apply_essentials_sku in azext_edge/edge/commands_edge.py (line 224) forces opcua.mode=Disabled for az iot ops create --sku Essentials, but az iot ops update --feature opcua.mode=Stable on an Essentials instance has no SKU awareness: it writes the feature and then runs check_default_opcua_template_needed → create_default_opcua_template, provisioning an OPC UA connector template on a SKU whose own --sku help text says it "does not support OPC UA". Since sku.name is already on the instance body that _update deep-copies (kwargs["instance"] / self.show(...)), the guard is cheap — reject or warn when instance.get("sku", {}).get("name") == "Essentials" and the requested mode is not Disabled. Same gap applies to az iot ops upgrade's backfill path in upgrade2.py.

Nits

  1. No test covers --sku against the preview blueprint. test_init_targets_instance_sku (azext_edge/tests/edge/orchestration/test_targets_unit.py, line 513) only exercises the default (GA) blueprint, while template_preview.py line 801/913 now carries its own sku parameter and binding. A --use-preview parametrization would lock the pairing the merge just created.

Checked and clean (recording the negatives so they are not re-derived)

  • az_client.py: the conflict was resolved as a union — the merged IoTOpsMgmtApiVersion keeps both V20261001 (Essentials) and V20260901_preview (dev), and DEFAULT_IOTOPS_MGMT_API_VERSION = V20261001 as both sides intended. Nothing dropped.
  • targets.py: the Essentials-side block instance.pop("sku", None); if self.sku: instance["sku"] = {"name": self.sku} is gone from the merged file, which looks like a dropped resolution but is correct. dev already carries the generated sku ARM parameter in both blueprints (template.py 1482/1597, template_preview.py 801/913: "[if(not(equals(parameters('sku'), null())), createObject('name', parameters('sku')), null())]"), so the python override is now redundant — and _handle_apply_targets still forwards "sku": self.sku, skipping it when None. The surviving plumbing (__init__ kwarg line 82, self.sku line 155, param_to_target line 280) is intact, and test_init_targets_instance_sku was updated to assert the ARM expression plus "sku" not in instance_parameters for the None case, so the test matches the new mechanism rather than silently passing.
  • params.py: --sku registration survived next to dev's new --use-preview, still get_enum_type(IoTOperationsSku).
  • test_init_int.py: the Essentials assertions survived — sku kwarg (254), instance_show/az iot ops list SKU checks (303-308), and the widened opcua_disabled condition (325-328).
  • The OPC UA hang risk on an Essentials create is covered. Neither blueprint puts a condition on opcUaConnectorTemplate, but targets.py lines 346-350 pop the resource from template.content["resources"] whenever instance_features["opcua"]["mode"] == "Disabled", and _apply_essentials_sku emits exactly that literal value — so --sku Essentials cannot deploy an unreconciled connector template. The pop is blueprint-agnostic, so it holds for --use-preview too.
  • Instances._update's backfill is gated on requested_opcua_mode not in (None, "Disabled"), so merely updating tags/description on an Essentials instance will not backfill a connector template.
  • Essentials-side contribution over dev is the expected 10 files / +122 lines (help, params, common.IoTOperationsSku, targets plumbing, _apply_essentials_sku, scenarios, tests) — no stray product change rode in on the merge.

Summary: the merge resolution is sound and nothing from either side was lost; the only substantive point is the create-time-only SKU enforcement in suggestion 1.

This is an automated review and may be incomplete.

dependabot Bot and others added 2 commits October 9, 2026 12:19
….1 to 4.38.2 (Azure#961)

chore(actions-deps): bump github/codeql-action/upload-sarif

Bumps [github/codeql-action/upload-sarif](https://github.com/github/codeql-action) from 4.38.1 to 4.38.2.
- [Release notes](https://github.com/github/codeql-action/releases)
- [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md)
- [Commits](github/codeql-action@1c5b675...2892aa5)

---
updated-dependencies:
- dependency-name: github/codeql-action/upload-sarif
  dependency-version: 4.38.2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Hangyi <hangyiwang@microsoft.com>

This branch has not been deployed

No deployments
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.

3 participants