Add composite actions so GitHub pipelines stop downloading the module - #449
Conversation
Consumers on GitHub Actions currently fetch scripts/Azure.Iot.Sdk.Test.psm1
from a raw.githubusercontent.com URL at run time and import it. The module they
get is whatever is on the branch in that URL, independent of the ref the caller
pinned, so a merge here changes another repository's pipeline with no way for
it to take the change deliberately.
Add composite actions that wrap the same module:
actions/provision-e2e-resources <-> vsts/templates/steps-create-azure-resources.yaml
actions/destroy-e2e-resources <-> vsts/templates/steps-destroy-azure-resources.yaml
actions/check-submodules <-> Test-SubmoduleConsistency
Running a composite action checks this repository out at the pinned ref, so the
action imports the module from its own checkout
(${{ github.action_path }}/../../scripts). There is no download, and the action
and the module are necessarily the same commit.
The provisioning and teardown actions are ports of the ones living in a
consumer repository, with three changes:
* Every input reaches the script through env instead of being interpolated
into the script body, so an input is always data and never code.
* config-cmdlet must resolve to a function exported by Azure.Iot.Sdk.Test,
and the numeric and enum inputs are validated, so a bad input fails with a
message naming it.
* -EnableADU is passed only when ADU is requested. It is not a parameter of
New-AzIotTestEnvironment on master, so passing it unconditionally fails
provisioning for callers that never asked for ADU.
tests/validate-actions.mjs checks the contracts: YAML parses, inputs are
declared and used in both directions (GitHub silently ignores an unknown key in
`with:`), no expression is interpolated into a script body, the module path
exists, and every module cmdlet call in an inline script names a cmdlet and
parameters the module actually declares. It runs in CI and locally with
`npm install --no-save js-yaml && node tests/validate-actions.mjs`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A workflow whose YAML is valid but whose expressions are not -- for instance a matrix context in `shell:`, which is evaluated before the matrix exists -- is rejected as a workflow file error and runs no job, so no check reports it as failing. Run actionlint over .github/workflows, and trigger the job on any change under that directory. Composite action.yml files are excluded: actionlint parses them as workflows and flags their 'runs' and 'inputs' keys. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved validation and input-handling issues remain in the reviewed actions and checks.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds pinned composite GitHub Actions for provisioning, teardown, and submodule checks without runtime module downloads.
Changes:
- Adds three composite actions and documentation.
- Adds YAML, PowerShell, and module-contract validation.
- Adds CI workflow coverage.
File summaries
| File | Summary |
|---|---|
tests/Validate-ActionScripts.ps1 |
Validates embedded PowerShell and module usage. |
tests/validate-actions.mjs |
Validates action metadata and scripts. |
actions/README.md |
Documents action usage and contracts. |
actions/provision-e2e-resources/action.yml |
Provisions resources and emits configuration. |
actions/destroy-e2e-resources/action.yml |
Deletes the resource group. |
actions/check-submodules/action.yml |
Runs submodule consistency checks. |
.gitignore |
Ignores Node dependencies. |
.github/workflows/validate-actions.yml |
Runs action validation in CI. |
Review details
Suppressed comments (5)
actions/provision-e2e-resources/action.yml:79
- The description says the enrollment group's issuer is emitted as
PROVISIONING_ROOT_CERT(_KEY), butNew-AzIotCSDKE2ETestConfigwrites the DPS root CA to those variables (scripts/Azure.Iot.Sdk.Test.psm1:2602-2607), while this action emits the group bootstrap leaf/full chain underIOT_DPS_GROUP_X509_CERTIFICATEandIOT_DPS_GROUP_X509_KEYbelow. Consumers following this description will use the wrong certificate and fail group CSR provisioning.
(New-AzIotTestEnvironment -DpsX509GroupEnrollmentDevices). A value > 0 creates the group
whose issuer certificate is emitted as PROVISIONING_ROOT_CERT(_KEY); required for the CSR
e2e, where the device presents a leaf signed by that CA and DPS issues its operational cert.
actions/provision-e2e-resources/action.yml:206
- These two boolean inputs are silently treated as false for every value other than the exact lowercase string
true; for example,enable-file-upload: trucompletes provisioning without storage notifications instead of rejecting the bad input. Since the action contract says enum inputs are validated, validate both values againsttrue|falseand fail with the input name before building the argument hashtable.
if ($env:AZ_IOT_ENABLE_CERT_MGMT -eq 'true') { $NewEnvArgs['EnableCertificateManagement'] = $true }
if ($env:AZ_IOT_ENABLE_FILE_UPLOAD -eq 'true') { $NewEnvArgs['EnableFileUpload'] = $true }
actions/provision-e2e-resources/action.yml:137
- A digits-only value above
Int32.MaxValuepasses the regex and then fails at the cast with PowerShell's generic conversion error, so the failure does not name the input as the action's validation contract promises. UseTryParseor an explicit range check and throw an error containing$Name.
return [int]$Value
tests/Validate-ActionScripts.ps1:56
- This
continuesilently accepts a misspelled or nonexistent module command. For example, changing an action call toNew-AzIotTestEnvironmntwould make the checker skip it and the workflow would fail only at runtime, despite this validator's documented contract to catch missing module cmdlets. Distinguish intentionally external commands from commands expected to come fromAzure.Iot.Sdk.Testinstead of ignoring every unknown command.
if (-not $CommandName) { continue }
if (-not $ModuleCommands.ContainsKey($CommandName)) { continue }
tests/Validate-ActionScripts.ps1:68
- The checker only inspects explicit
CommandParameterAstnodes, but the provisioning action passes mostNew-AzIotTestEnvironmentparameters through$NewEnvArgssplatting (and the submodule action does the same with$CheckArgs). A drift such as a misspelled hashtable key therefore passes validation, so the advertised parameter-drift check does not cover the actual call pattern used by these actions.
foreach ($Element in $Call.CommandElements) {
if ($Element -isnot [System.Management.Automation.Language.CommandParameterAst]) { continue }
$ParameterName = $Element.ParameterName
# Accept a unique prefix, exactly as PowerShell's own binder does.
$Matched = @($Parameters.Keys | Where-Object { $_ -like "$ParameterName*" })
if ($Matched.Count -eq 0) {
$Problems.Add("$Name : $CommandName has no parameter -$ParameterName")
} elseif ($Matched.Count -gt 1 -and $Matched -notcontains $ParameterName) {
$Problems.Add("$Name : -$ParameterName is ambiguous for $CommandName ($($Matched -join ', '))")
- Files reviewed: 7/8 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Two problems raised in review. Get-Command -Name takes a WILDCARD, so a config-cmdlet of 'New-AzIot*' returned 7 functions. The guard then compared an ARRAY to the module name, which is falsy and passed, and `& $ConfigCmdlet` was handed an array. Filter on an exact name and require exactly one match. Validate-ActionScripts.ps1 inspected only CommandParameterAst nodes, so a call passing everything by splat -- which is how the action calls New-AzIotTestEnvironment -- was not checked at all. Resolve splatted hashtable variables statically: keys from literal hashtable assignments and from literal index assignments. Keys built dynamically still cannot be resolved; this narrows the blind spot rather than closing it. That check correctly flags EnableADU, which the action adds only after testing for it at runtime. A per-name, greppable annotation (`# validate-actions: allow-parameter EnableADU`) marks that one case instead of weakening the check. Verified: the splat check catches a misspelled key in both the literal hashtable and an index assignment, and the annotation does not cover any other key. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Validation gaps allow nonexistent module commands, invalid config functions, and some script-body expressions to evade preflight checks.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
actions/provision-e2e-resources/action.yml:138
- An all-digit value larger than
Int32.MaxValuepasses the numeric check and then fails at this cast with a generic overflow error, so the invalid input is not rejected with the promised input-specific message. Include theInt32range conversion in the validation (for example withTryParse) before returning the count.
actions/provision-e2e-resources/action.yml:162 - Exact-name lookup still accepts any exported module function, including
New-AzureResourceGroupName, which does not accept the-TestEnvInfo,-Target, and-OutFilearguments used below. Such an input passes this preflight, provisions Azure resources, and only then fails during config rendering. Validate the selected function's parameter set here or restrict this input to the module's config-renderer functions before provisioning starts.
tests/Validate-ActionScripts.ps1:123 - This guard only validates parameter names for commands already present in the module. A removed or misspelled call such as
New-AzIotTestEnvironmntis absent from$ModuleCommands, hits thiscontinue, and the validator succeeds—even though detecting nonexistent module cmdlets is one of its stated contracts. Make intended module calls identifiable (for example, module-qualify them or declare the expected module command names) and report an error when an expected command is missing.
- Files reviewed: 7/8 changed files
- Comments generated: 1
- Review effort level: Balanced
Ewerton Scaboro da Silva (ewertons)
left a comment
There was a problem hiding this comment.
Approved
Raised in review: the script-body check matched `${{ ... }}` with a `[^}]*`
body, so an expression containing a brace -- `${{ format('{0}', inputs.x) }}`
-- matched nothing and passed. Confirmed: the old pattern returned no match for
exactly that string.
The invariant for a script body is that it contains no expression at all, so
count `${{` openers directly. A body whose expression this script cannot parse
now fails rather than passes, and the message falls back to the opener count.
Two related fixes in the same pattern:
* expression matching is non-greedy up to the first `}}`, so an expression
containing braces is reported with its text, and the MODULE env path strip
handles one too.
* an input reference is matched anywhere in an expression rather than only at
its start, so an input used inside a call is not misreported as declared
but never used.
Verified: the reported bypass is caught; an unterminated opener is caught; an
undeclared input inside format() is caught; and an input used only inside
format() is not reported unused.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses the suppressed findings from review. Boolean inputs were compared to the literal 'true', so 'enable-file-upload: tru' provisioned without file upload instead of failing. Both booleans now go through Get-BooleanInput, which accepts only 'true' or 'false' and names the input otherwise. Counts used a '^\d+$' regex and then cast to [int], so a digits-only value above Int32.MaxValue passed the check and failed at the cast with a generic conversion error. Get-CountInput now uses [int]::TryParse and reports the input name and the accepted range. config-cmdlet only had to be exported by the module, so New-AzureResourceGroupName passed, Azure resources were provisioned, and the run failed afterwards while rendering the config. It must now also declare -TestEnvInfo, -Target and -OutFile. Matching is by unique prefix, exactly as PowerShell's binder does, because New-AzIotPythonSdkSampleConfig declares -TargetEnvironment and binds -Target to it: a ContainsKey test would have rejected a renderer that works today. Verified all six renderers are accepted and New-AzureResourceGroupName, New-AzIotTestEnvironment and Test-SubmoduleConsistency are rejected. Validate-ActionScripts.ps1 skipped every command it did not find in the module, so a misspelled New-AzIotTestEnvironmnt was silently accepted -- defeating one of its stated contracts. A command must now resolve as a module command, a function the script defines, a command on this machine, or a named external tool; anything else is an error. The dps-x509-group-enrollment-devices description said the group's issuer is emitted as PROVISIONING_ROOT_CERT(_KEY). It is not: the config cmdlets write the DPS ROOT CA there, while the group's issuer is an intermediate signed by it, and this action emits the bootstrap identity as IOT_DPS_GROUP_X509_*. A consumer following the old text would have used the wrong certificate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the suppressed findings from the two Copilot reviews in 350f3a5. All confirmed before fixing.
The splat-coverage finding was already fixed in 1f1b29c. |
GitHub Actions consumers fetch
scripts/Azure.Iot.Sdk.Test.psm1from a raw.githubusercontent.com URL at run time. The module they get is whatever is on the branch in that URL, independent of the ref the caller pinned, so a merge here changes another repository's pipeline with no way for it to take the change deliberately.Add composite actions wrapping the same module:
actions/provision-e2e-resourcesvsts/templates/steps-create-azure-resources.yamlactions/destroy-e2e-resourcesvsts/templates/steps-destroy-azure-resources.yamlactions/check-submodulesTest-SubmoduleConsistencyRunning a composite action checks this repository out at the pinned ref, so the action imports the module from its own checkout (
${{ github.action_path }}/../../scripts). No download, and action and module are necessarily the same commit.Provisioning and teardown are ports of the actions in a consumer repository, with three changes:
env:instead of being interpolated into the script body, so an input is always data and never code.config-cmdletmust resolve to a function exported byAzure.Iot.Sdk.Test; numeric and enum inputs are validated, so a bad input fails naming itself.-EnableADUis passed only when ADU is requested. It is not a parameter ofNew-AzIotTestEnvironmenton master (it exists only on the unmergedewertons/adu-provisioning), so passing it unconditionally fails provisioning for callers that never asked for ADU.tests/validate-actions.mjschecks the contracts: YAML parses; inputs are declared and used in both directions (GitHub silently ignores an unknown key inwith:); no expression is interpolated into a script body; the module path exists; and every module cmdlet call in an inline script names a cmdlet and parameters the module declares.Verification
Run locally, all passing:
Negative tests confirm the checks fail on: a drifted cmdlet parameter (reproduces the
-EnableADUbug above), and an input interpolated into a script body.Not verified: an actual provisioning run, which needs an Azure subscription and a service principal.
Follow-ups (not in this PR)
@v1; the repo currently has no version tags.