Skip to content

CustomModule fixes and Copilot repo guidance#560

Merged
gaelcolas merged 11 commits into
mainfrom
f/copilot+custommodule
Apr 23, 2026
Merged

CustomModule fixes and Copilot repo guidance#560
gaelcolas merged 11 commits into
mainfrom
f/copilot+custommodule

Conversation

@gaelcolas
Copy link
Copy Markdown
Owner

@gaelcolas gaelcolas commented Apr 20, 2026

Pull Request

  • Make Add-Sample and New-SamplerPipeline resolve Plaster's culture-aware manifest-path helper at runtime so they work with both Plaster v2.x
    (Get-PlasterManifestPathForCulture) and v1.x (GetPlasterManifestPathForCulture).
  • Fix New-SampleModule -Features CustomModule scaffolding so supplying features + required parameters is fully non-interactive (no prompts for
    features the user did not ask for, no GitHub-owner prompt when GitHub/Azure Pipelines were not selected).
  • Plaster template fixes: correct Features choice values (azurepipelines, vscode, github, codecov), repair broken prompt conditions (they used
    contradictory Features.Count checks), gate GitVersion.yml on the gitversion feature (not git), and rewrite GitHubOwner condition.
  • Add matching -Features values to the New-SampleModule ValidateSet and document the CustomModule / MainGitBranch parameters.
  • Update integration tests for the corrected template behavior (git-only, Build-only, dsccommunity contexts).
  • Add repo-wide GitHub Copilot guidance under .github/: copilot-instructions.md, per-area instructions/.instructions.md files,
    agents/sampler-maintainer.md, and skills/validate-changes. The instructions mandate going through ./build.ps1, document cross-platform / PS5.1+PS7
    support, the -ErrorAction 'Ignore' preference, how to tee long builds to a log file to avoid agent-shell hangs, and how to extract real failures
    from output/testResults/NUnitXml_
    .xml (Pester) and output/testResults/DscTestObject_DscTest_*.xml (HQRM CliXml).
  • Update comment-based help for Add-Sample / New-SamplerPipeline (Plaster v1/v2 note) and New-SampleModule (non-interactive -Features example).
  • Expand Sampler/WikiSource/Getting-started.md with the full -Features matrix and a non-interactive-scaffolding callout.

This change is Reviewable

@raandree
Copy link
Copy Markdown
Contributor

Hidden auto-switch of ModuleType to CustomModule

In Sampler/Public/New-SampleModule.ps1:

if ($PSBoundParameters.ContainsKey('Features') -and -not $PSBoundParameters.ContainsKey('ModuleType'))
{
    $ModuleType = 'CustomModule'
}

This silently overrides the documented default (ModuleType = 'SimpleModule'). A caller who passes -Features expecting their current SimpleModule defaults to still apply will get a different scaffold with no diagnostic, and the behavior isn't discoverable from the parameter help — it's only mentioned in a tip box in the wiki page.

I'd prefer we nail this down in this PR rather than in a follow-up, because once it ships the auto-switch becomes a compatibility constraint: a later change that adds a warning or throws is itself a breaking change.

Two options, either is fine by me:

  1. Make it explicit (preferred). Throw a terminating error when -Features is supplied with a non-CustomModule ModuleType, since the wiki already states that Features is ignored by the other presets. That turns a silent no-op into a clear contract:

    if ($PSBoundParameters.ContainsKey('Features') -and $ModuleType -ne 'CustomModule' -and $PSBoundParameters.ContainsKey('ModuleType'))
    {
        throw "The -Features parameter is only honored when -ModuleType is 'CustomModule'. Either omit -ModuleType (it will default to 'CustomModule' when -Features is supplied) or set -ModuleType 'CustomModule' explicitly."
    }
    
    if ($PSBoundParameters.ContainsKey('Features') -and -not $PSBoundParameters.ContainsKey('ModuleType'))
    {
        Write-Verbose -Message "-Features was supplied without -ModuleType; defaulting -ModuleType to 'CustomModule'."
        $ModuleType = 'CustomModule'
    }
  2. Keep the auto-switch but make it visible and documented. Emit Write-Verbose (or Write-Warning) when the switch happens, and add a note to the .PARAMETER Features comment-based help explaining that supplying -Features without -ModuleType defaults -ModuleType to CustomModule.

Happy to approve once this is resolved; the remaining review items will be tracked as follow-up issues.

@gaelcolas
Copy link
Copy Markdown
Owner Author

I think you're right that the ModuleType shouldn't be changed, and I think it does not really conflicts...
I think if we remove this, and depending on the template, we can select a simple module, and add specific features...

Let's see if the build completes.

@gaelcolas
Copy link
Copy Markdown
Owner Author

Scratch that, I forgot how Plaster works.
I'm not sure which one I forgot between option 1 and 2.

@gaelcolas gaelcolas force-pushed the f/copilot+custommodule branch from c3e408b to 8d91766 Compare April 21, 2026 05:00
@gaelcolas
Copy link
Copy Markdown
Owner Author

I've implemented 2 for now because I think in the longer term we should support features WITH any template, and for that we need to change the condition logic within the template (but I'd rather do that in another PR).
If this one is merged in, I might attempt the change before the next full release.

@raandree
Copy link
Copy Markdown
Contributor

raandree commented Apr 21, 2026

:lgtm:

Tracked the remaining review items as follow-up issues:

Copy link
Copy Markdown
Collaborator

@johlju johlju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johlju reviewed 17 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on gaelcolas).


Sampler/Templates/Sampler/plasterManifest.xml line 119 at r1 (raw file):

                  prompt="Do you want to add configuration for GitVersion to handle automatic versioning for your project?"
                  default="0"
                  condition="$PLASTER_PARAM_UseGit -eq 'true' -and $PLASTER_PARAM_ModuleType -notin @('CustomModule')" >

Should we not allow GitVersion question in CustomModule?

Code quote:

-and $PLASTER_PARAM_ModuleType -notin @('CustomModule')

Sampler/Templates/Sampler/plasterManifest.xml line 151 at r1 (raw file):

                  default="MyOrgName"
                  prompt="What is the name of the GitHub owner (personal or organization account) that will publish the module?"
                  condition="($PLASTER_PARAM_ModuleType -eq 'CustomModule' -and ($PLASTER_PARAM_Features -contains 'github' -or $PLASTER_PARAM_Features -contains 'All')) -or ($PLASTER_PARAM_UseGitHub -eq 'true' -and $PLASTER_PARAM_ModuleType -notin @('dsccommunity', 'SimpleModule_NoBuild', 'CustomModule')) -or $PLASTER_PARAM_ModuleType -in @('CompleteSample')" />

THis condition seems to exclude CustomModule if the user answered yes on using GitHub question above.

Code quote:

condition="($PLASTER_PARAM_ModuleType -eq 'CustomModule' -and ($PLASTER_PARAM_Features -contains 'github' -or $PLASTER_PARAM_Features -contains 'All')) -or ($PLASTER_PARAM_UseGitHub -eq 'true' -and $PLASTER_PARAM_ModuleType -notin @('dsccommunity', 'SimpleModule_NoBuild', 'CustomModule')) -or $PLASTER_PARAM_ModuleType -in @('CompleteSample')"

Sampler/Public/New-SamplerPipeline.ps1 line 122 at r1 (raw file):

                {
                    throw "Unable to locate Plaster's manifest-path-for-culture helper. Expected 'Get-PlasterManifestPathForCulture' (Plaster 2.x) or 'GetPlasterManifestPathForCulture' (Plaster 1.x)."
                }

Suggest maybe have this as a private function as it is repeated in at least two public functions. That would avoid chaning in one place and miss the other(s).

Code quote:

                $manifestPathCommand = Get-Command -Name 'Get-PlasterManifestPathForCulture' -ErrorAction 'Ignore'

                if (-not $manifestPathCommand)
                {
                    $manifestPathCommand = Get-Command -Name 'GetPlasterManifestPathForCulture' -ErrorAction 'Ignore'
                }

                if (-not $manifestPathCommand)
                {
                    throw "Unable to locate Plaster's manifest-path-for-culture helper. Expected 'Get-PlasterManifestPathForCulture' (Plaster 2.x) or 'GetPlasterManifestPathForCulture' (Plaster 1.x)."
                }

@johlju
Copy link
Copy Markdown
Collaborator

johlju commented Apr 21, 2026

@gaelcolas looks good, just some comments (see above)

I just skimmed the AI stuff, guessing you have run with those so didn't bother digging in - but they were a somewhat verbose so they probably can be condensed since an AI reads them.

I usually add this ai-instruction-authoring.instructions.md to .github/instructions that seem to help a lot (thanks to @PlagueHO for inspiration).

---
applyTo: "{.github/instructions/*.md,.github/prompts/*.md,.github/skills/*.md,**/AGENTS.md,.github/copilot-instructions.md}"
---

# AI Instruction Authoring

These files are AI-only. No human-facing prose, tutorials, or rationale. Meant to
reduce token usage, eliminate conflicting information and ensure precise, clear,
concise guidance.

## Rules

- Write short imperative directives. Bullet lists over prose.
- Remove filler words, redundant qualifiers, repeated context.
- Omit *why* unless the reason changes behaviour.
- Check existing instructions before adding rules. Update existing rules on conflict; never duplicate.
- Use narrowest `applyTo` glob possible. Never `**/*` when a specific
  path suffices. `ApplyTo` attribute must be a string, never an array.
- Start each file, except copilot-instructions.md and **/AGENTS.md, with YAML
  frontmatter `applyTo`
- Use `##`/`###` headings, `-` bullets, backticks for code tokens, fenced blocks for multi-line examples.
- No bold/italic emphasis, conversational tone, or verbose examples.

@gaelcolas
Copy link
Copy Markdown
Owner Author

Saw your comments @johlju and I'll address them in a subsequent PR.
It turns out the impact is minimal in the current state, because of how the function is written.
But I need to come up with a more elegant condition system and do some clean up.
As for the instructions, I'll play with them, thanks.

@gaelcolas gaelcolas merged commit 4b37ecc into main Apr 23, 2026
11 of 12 checks passed
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