feat(Pull-SDLC): replace -SetupGitHubSsh with -SetupGit#172
Closed
MarkMichaelis wants to merge 1 commit into
Closed
feat(Pull-SDLC): replace -SetupGitHubSsh with -SetupGit#172MarkMichaelis wants to merge 1 commit into
MarkMichaelis wants to merge 1 commit into
Conversation
The previous -SetupGitHubSsh switch was wrong on two axes:
1. Protocol is not the script's business. Whether a user connects to
GitHub over HTTPS, SSH-22, or SSH-443 is a per-machine choice and
must not be silently rewritten by an instructions-sync script.
2. GitHub-specific framing was too narrow. The pieces a fresh repo
actually wants pre-configured are .gitignore and .gitattributes --
git configuration, not GitHub configuration.
This change replaces -SetupGitHubSsh with -SetupGit:
- Idempotently creates .gitattributes with a minimal line-ending
normalization baseline (`* text=auto eol=lf`) when missing.
Never overwrites an existing file.
- Union-merges the upstream .gitignore into the consumer copy via
the existing Merge-FileFromUpstream code path.
- Does NOT touch SSH keys, ssh-agent, machine-wide git settings,
gh CLI configuration, or URL rewrites.
Removed (breaking; -SetupGitHubSsh was recent, from #164):
- -SetupGitHubSsh and -SkipKeyUpload parameters
- Invoke-SetupGitHubSsh function
- Helpers: Get-GitHubSshKeyPath, Test-GitHubSshAgentRunning,
Test-GhInstalled, Get-GitConfigAllValues, Get-GhGitProtocol,
Add-GitConfigValueIfMissing
- 6 obsolete Pester Describe blocks covering the removed helpers
Added tests:
- Invoke-SetupGit behavior (5 tests): creation, non-overwrite of
existing files, ref forwarding to Merge-FileFromUpstream,
idempotency.
- Contract-guard regression tests (4 tests) asserting the script
no longer declares -SetupGitHubSsh/-SkipKeyUpload, no longer
defines Invoke-SetupGitHubSsh, and contains no code references
to global git config, gh config set, ssh-keygen, or url.insteadOf
-- so future commits cannot quietly re-add protocol mutations.
Closes #171
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Closing in favor of #161 (issue #160), which was opened ~4 hours before this PR and tackles the same scope with a substantially more thoughtful design:
This PR was built in a session that didn't know about #161. Apologies for the duplicate work. The minimal The orthogonal "remove unsolicited drift nudge" fix is preserved in #170 (parent of this PR's branch) and will land independently. Closes nothing -- issue #171 is being closed separately as superseded by #160. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the misguided
-SetupGitHubSshswitch (added in #164, partially walked back in #170) with a focused-SetupGitthat does what a per-repo sync script should actually do: scaffold repo-level git files. No more touching SSH keys,ssh-agent, machine-wide git settings, theghCLI, or URL rewrites -- protocol choices stay with the user.Per the discussion on #169, the old switch was wrong on two axes:
.gitignoreand.gitattributes-- git configuration, not GitHub configuration. Hence-SetupGit, not-SetupGitHub.What
-SetupGitdoes.gitattributes: creates the file with a minimal line-ending normalization baseline (* text=auto eol=lf) when missing. Never overwrites an existing file..gitignore: union-merges upstream entries into the consumer copy via the existingMerge-FileFromUpstreamcode path (the same one the normal sync uses).Every step prints
[skip]or[add]so the user sees exactly what changed.Removed (breaking)
-SetupGitHubSshwas recent (#164) and the broader direction was already being walked back. Clean removal beats a deprecation alias:-SetupGitHubSshand-SkipKeyUploadparametersInvoke-SetupGitHubSshfunctionGet-GitHubSshKeyPath,Test-GitHubSshAgentRunning,Test-GhInstalled,Get-GitConfigAllValues,Get-GhGitProtocol,Add-GitConfigValueIfMissingDescribeblocks covering the removed helpersTests added
Invoke-SetupGitbehavior (5 tests): creates.gitattributeswith the expected baseline when missing, leaves an existing.gitattributesalone, forwards the upstream ref correctly toMerge-FileFromUpstream, does not invokessh-keygen/gh/git config --global(contract guard), is idempotent.-SetupGitHubSsh/-SkipKeyUpload, no longer definesInvoke-SetupGitHubSsh, and contains no code references togit config --global,gh config set,ssh-keygen, orurl.insteadOf. These are the real ratchet -- they fail loudly if anyone later re-adds protocol-level mutations.Diff stats
2 files changed, +155 / -362. Net deletion of ~200 lines.
Stacked on
This PR is stacked on #170 (which removes the unsolicited drift nudge). Merge #170 first, then this PR will rebase cleanly onto
main.Verification
\\powershell
Invoke-Pester -Path .\Pull-SDLC.ai.Tests.ps1 -FullNameFilter '-SetupGit','Invoke-SetupGit','-SetupGitHubSsh removed','post-sync output'
\\
Local result: 10 passed, 0 failed. Broader subset (31 tests across
ConvertTo-GitHubRepoSlug,Resolve-OpenSyncPRAction,Test-IsCiEnvironment,Test-IsUpstreamRepo, plus all new tests): 31 passed, 0 failed.Closes #171