fix(init): fix handling of gitlab repos#7945
Conversation
📊 Benchmark resultsComparing with 30d4ef0
|
| provider: 'manual', | ||
| repo_path: repoPath, | ||
| provider: repoData.provider ?? 'manual', | ||
| repo_path: repoData.repo ?? repoPath, |
There was a problem hiding this comment.
[small bug] I think this is dead code- we'll be prompting users for an SSH that never gets send via network request ie repoData.repo ("owner/name")
Should we do something like so only the manual case will prompt the user
const repoPath = repoData.repo ?? (await getRepoPath({ repoData }))
repo_path: repoPath,
* ci: create integration test sites in testing account This [mechanism already exists](https://github.com/netlify/cli/blob/b80b98f85929803fc35a08458c9327dc7ef63de0/tests/integration/utils/create-live-test-site.ts#L22-L36) but wasn't being used here, so it was falling back to the first account the user has access to that is returned by the accounts API. The `netlify-integration-testing` account is properly configured to host our various integration test sites, e.g. to avoid being rate limited. * ci: try DEBUG_TESTS=1 * Revert "ci: try DEBUG_TESTS=1" This reverts commit 2d186bf. * chore: bump deploy integration test timeout and concurrency
* feat: begin integrating `@netlify/dev` * chore: add deps * fix(deps): bump netlify packages to dedupe with @netlify/dev * chore: update deps * chore: fix lint issue * chore: add debug logging * chore: add comment Co-authored-by: Philippe Serhal <philippe.serhal@netlify.com> * chore: cap site name length in tests * chore: update snapshot --------- Co-authored-by: Philippe Serhal <philippe.serhal@netlify.com>
* feat: deprecate sites:create-template * chore: format * chore: lint * docs: build docs * fix: claude says this is how we fix the PR title lint issue * fix: lol now claude says this will work * remove this * remove unneeded issuePrefixes that claude added * fix: remove a bit more dead code * fix: remove sites:create-template placeholder cmd --------- Co-authored-by: Philippe Serhal <philippe.serhal@netlify.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request changes repo handling in src/utils/init/config-manual.ts: repoPath resolution now uses repoData.repo as an override before calling getRepoPath({ repoData }), and repo.provider is set from repoData.provider with a fallback to 'manual' instead of being hardcoded. It also adds two unit test files: tests/unit/utils/get-repo-data.test.ts (parsing and provider inference for GitHub/GitLab SSH and HTTPS remotes, remote selection, and error cases) and tests/unit/utils/init/config-manual.test.ts (config-manual initialization flows with mocked prompts and Netlify API interactions). Estimated code review effort🎯 2 (Simple) | ⏱️ ~13 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/utils/get-repo-data.test.ts`:
- Around line 8-119: The tests are self-fulfilling because they construct
RepoData literals and assert those same values; update each spec to call the
real getRepoData function instead of building RepoData objects so
parsing/normalization is exercised. Replace the mockRepoData literals with calls
to getRepoData(...) (or the exported function that returns RepoData) using the
raw git URL/branch/name inputs from each case (e.g.,
'git@github.com:ownername/test.git', 'https://gitlab.com/...', custom host,
etc.), then assert on returnedRepo.httpsUrl, returnedRepo.provider and
returnedRepo.repo to validate behavior. Ensure you import getRepoData and keep
the same expected strings in assertions.
In `@tests/unit/utils/init/config-manual.test.ts`:
- Around line 41-45: The test is failing due to an unsafe direct cast of the
Netlify `config` property; locate the object literal assigned to
`netlify.config` in tests/unit/utils/init/config-manual.test.ts and change the
cast so the provided `{ plugins: [] }` is first cast to `unknown` and then to
the required `CachedConfig['config']` (or BaseCommand['netlify']['config']) type
instead of a direct cast; this preserves the intended test shape while
satisfying TypeScript's structural checks for `build` and other required
properties referenced by `CachedConfig['config']`.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/utils/init/config-manual.tstests/unit/utils/get-repo-data.test.tstests/unit/utils/init/config-manual.test.ts
| describe('getRepoData', () => { | ||
| describe('RepoData structure for different Git providers', () => { | ||
| it('should construct correct httpsUrl for GitHub SSH URLs', () => { | ||
| const mockRepoData: RepoData = { | ||
| name: 'test', | ||
| owner: 'ownername', | ||
| repo: 'ownername/test', | ||
| url: 'git@github.com:ownername/test.git', | ||
| branch: 'main', | ||
| provider: 'github', | ||
| httpsUrl: 'https://github.com/ownername/test', | ||
| } | ||
|
|
||
| expect(mockRepoData.httpsUrl).toBe('https://github.com/ownername/test') | ||
| expect(mockRepoData.provider).toBe('github') | ||
| expect(mockRepoData.repo).toBe('ownername/test') | ||
| }) | ||
|
|
||
| it('should construct correct httpsUrl for GitLab SSH URLs', () => { | ||
| const mockRepoData: RepoData = { | ||
| name: 'test', | ||
| owner: 'ownername', | ||
| repo: 'ownername/test', | ||
| url: 'git@gitlab.com:ownername/test.git', | ||
| branch: 'main', | ||
| provider: 'gitlab', | ||
| httpsUrl: 'https://gitlab.com/ownername/test', | ||
| } | ||
|
|
||
| expect(mockRepoData.httpsUrl).toBe('https://gitlab.com/ownername/test') | ||
| expect(mockRepoData.provider).toBe('gitlab') | ||
| expect(mockRepoData.repo).toBe('ownername/test') | ||
| }) | ||
|
|
||
| it('should construct correct httpsUrl for GitHub HTTPS URLs', () => { | ||
| const mockRepoData: RepoData = { | ||
| name: 'test', | ||
| owner: 'ownername', | ||
| repo: 'ownername/test', | ||
| url: 'https://github.com/ownername/test.git', | ||
| branch: 'main', | ||
| provider: 'github', | ||
| httpsUrl: 'https://github.com/ownername/test', | ||
| } | ||
|
|
||
| expect(mockRepoData.httpsUrl).toBe('https://github.com/ownername/test') | ||
| expect(mockRepoData.provider).toBe('github') | ||
| expect(mockRepoData.repo).toBe('ownername/test') | ||
| }) | ||
|
|
||
| it('should construct correct httpsUrl for GitLab HTTPS URLs', () => { | ||
| const mockRepoData: RepoData = { | ||
| name: 'test', | ||
| owner: 'ownername', | ||
| repo: 'ownername/test', | ||
| url: 'https://gitlab.com/ownername/test.git', | ||
| branch: 'main', | ||
| provider: 'gitlab', | ||
| httpsUrl: 'https://gitlab.com/ownername/test', | ||
| } | ||
|
|
||
| expect(mockRepoData.httpsUrl).toBe('https://gitlab.com/ownername/test') | ||
| expect(mockRepoData.provider).toBe('gitlab') | ||
| expect(mockRepoData.repo).toBe('ownername/test') | ||
| }) | ||
|
|
||
| it('should use host as provider for unknown Git hosts', () => { | ||
| const mockRepoData: RepoData = { | ||
| name: 'test', | ||
| owner: 'user', | ||
| repo: 'user/test', | ||
| url: 'git@custom-git.example.com:user/test.git', | ||
| branch: 'main', | ||
| provider: 'custom-git.example.com', | ||
| httpsUrl: 'https://custom-git.example.com/user/test', | ||
| } | ||
|
|
||
| expect(mockRepoData.httpsUrl).toBe('https://custom-git.example.com/user/test') | ||
| expect(mockRepoData.provider).toBe('custom-git.example.com') | ||
| expect(mockRepoData.repo).toBe('user/test') | ||
| }) | ||
| }) | ||
|
|
||
| describe('provider field mapping', () => { | ||
| it('should map github.com to "github" provider', () => { | ||
| const mockRepoData: RepoData = { | ||
| name: 'test', | ||
| owner: 'user', | ||
| repo: 'user/test', | ||
| url: 'git@github.com:user/test.git', | ||
| branch: 'main', | ||
| provider: 'github', | ||
| httpsUrl: 'https://github.com/user/test', | ||
| } | ||
|
|
||
| expect(mockRepoData.provider).toBe('github') | ||
| }) | ||
|
|
||
| it('should map gitlab.com to "gitlab" provider', () => { | ||
| const mockRepoData: RepoData = { | ||
| name: 'test', | ||
| owner: 'user', | ||
| repo: 'user/test', | ||
| url: 'git@gitlab.com:user/test.git', | ||
| branch: 'main', | ||
| provider: 'gitlab', | ||
| httpsUrl: 'https://gitlab.com/user/test', | ||
| } | ||
|
|
||
| expect(mockRepoData.provider).toBe('gitlab') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Tests are self-fulfilling; exercise getRepoData instead.
These cases construct RepoData literals and assert the same values, so they don’t validate parsing/normalization behavior. Consider driving the tests through getRepoData (with mocked repo info) and asserting its output to catch regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/utils/get-repo-data.test.ts` around lines 8 - 119, The tests are
self-fulfilling because they construct RepoData literals and assert those same
values; update each spec to call the real getRepoData function instead of
building RepoData objects so parsing/normalization is exercised. Replace the
mockRepoData literals with calls to getRepoData(...) (or the exported function
that returns RepoData) using the raw git URL/branch/name inputs from each case
(e.g., 'git@github.com:ownername/test.git', 'https://gitlab.com/...', custom
host, etc.), then assert on returnedRepo.httpsUrl, returnedRepo.provider and
returnedRepo.repo to validate behavior. Ensure you import getRepoData and keep
the same expected strings in assertions.
| netlify: { | ||
| api: mockApi as NetlifyAPI, | ||
| cachedConfig: { configPath: '/test/netlify.toml' } as BaseCommand['netlify']['cachedConfig'], | ||
| config: { plugins: [] } as BaseCommand['netlify']['config'], | ||
| repositoryRoot: '/test', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "(config-manual\.test\.ts|base-command\.ts)"Repository: netlify/cli
Length of output: 127
🏁 Script executed:
cat -n tests/unit/utils/init/config-manual.test.ts | sed -n '35,50p'Repository: netlify/cli
Length of output: 608
🏁 Script executed:
head -100 src/commands/base-command.ts | cat -nRepository: netlify/cli
Length of output: 4256
🏁 Script executed:
cat -n src/commands/base-command.ts | sed -n '100,250p'Repository: netlify/cli
Length of output: 6552
🏁 Script executed:
cat -n src/commands/types.tsRepository: netlify/cli
Length of output: 108
🏁 Script executed:
rg -t ts "type NetlifyOptions|interface NetlifyOptions" --max-count 5Repository: netlify/cli
Length of output: 109
🏁 Script executed:
cat -n src/commands/types.d.tsRepository: netlify/cli
Length of output: 1910
🏁 Script executed:
rg -t ts "type NormalizedCachedConfigConfig|interface NormalizedCachedConfigConfig" -A 15Repository: netlify/cli
Length of output: 14867
🏁 Script executed:
rg -t ts "type CachedConfig|interface CachedConfig" src/lib/build.js -A 20Repository: netlify/cli
Length of output: 111
🏁 Script executed:
fd build src/lib -t f | head -10Repository: netlify/cli
Length of output: 71
🏁 Script executed:
rg "type CachedConfig|interface CachedConfig" --max-count 5Repository: netlify/cli
Length of output: 154
🏁 Script executed:
rg "export interface CachedConfig" -A 30 src/lib/build.tsRepository: netlify/cli
Length of output: 1068
🏁 Script executed:
rg "export interface CachedConfig" -A 60 src/lib/build.ts | head -80Repository: netlify/cli
Length of output: 2080
🏁 Script executed:
rg "export interface CachedConfig" -A 150 src/lib/build.ts | grep -A 5 "plugins"Repository: netlify/cli
Length of output: 499
🏁 Script executed:
cat -n tests/unit/utils/init/config-manual.test.ts | head -80Repository: netlify/cli
Length of output: 3052
🏁 Script executed:
cat tsconfig.json | head -50Repository: netlify/cli
Length of output: 94
🏁 Script executed:
cat tsconfig.base.json | head -80Repository: netlify/cli
Length of output: 403
🏁 Script executed:
cd /tmp && cat > check_types.ts << 'EOF'
import type { CachedConfig } from '/repo/src/lib/build'
import type BaseCommand from '/repo/src/commands/base-command'
// This is what the test is trying to do
const testConfig: BaseCommand['netlify']['config'] = { plugins: [] }
EOF
npx tsc --noEmit check_types.ts 2>&1Repository: netlify/cli
Length of output: 300
🏁 Script executed:
rg "export interface CachedConfig" -A 200 src/lib/build.ts | tail -50Repository: netlify/cli
Length of output: 1462
Fix the failing type assertion on config.
Line 44 attempts an invalid direct cast. The config property requires a CachedConfig['config'] object with required properties like build, but only provides { plugins: [] }. Use a two-step cast through unknown:
Proposed fix
- config: { plugins: [] } as BaseCommand['netlify']['config'],
+ config: { plugins: [] } as unknown as BaseCommand['netlify']['config'],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| netlify: { | |
| api: mockApi as NetlifyAPI, | |
| cachedConfig: { configPath: '/test/netlify.toml' } as BaseCommand['netlify']['cachedConfig'], | |
| config: { plugins: [] } as BaseCommand['netlify']['config'], | |
| repositoryRoot: '/test', | |
| netlify: { | |
| api: mockApi as NetlifyAPI, | |
| cachedConfig: { configPath: '/test/netlify.toml' } as BaseCommand['netlify']['cachedConfig'], | |
| config: { plugins: [] } as unknown as BaseCommand['netlify']['config'], | |
| repositoryRoot: '/test', |
🧰 Tools
🪛 GitHub Actions: Typecheck
[error] 44-44: TypeScript error TS2352: Conversion of type '{ plugins: never[]; }' to type 'NormalizedCachedConfigConfig' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first. (During npm run typecheck/tsc)
🪛 GitHub Check: typecheck
[failure] 44-44:
Conversion of type '{ plugins: never[]; }' to type 'NormalizedCachedConfigConfig' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/utils/init/config-manual.test.ts` around lines 41 - 45, The test
is failing due to an unsafe direct cast of the Netlify `config` property; locate
the object literal assigned to `netlify.config` in
tests/unit/utils/init/config-manual.test.ts and change the cast so the provided
`{ plugins: [] }` is first cast to `unknown` and then to the required
`CachedConfig['config']` (or BaseCommand['netlify']['config']) type instead of a
direct cast; this preserves the intended test shape while satisfying
TypeScript's structural checks for `build` and other required properties
referenced by `CachedConfig['config']`.
serhalp
changed the title
Rewrite get-repo-data tests to drive the real getRepoData function with mocked git deps so parsing and provider mapping are actually exercised, and fix the unsafe direct cast on netlify.config in the config-manual test by routing it through unknown. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 participants
🎉 Thanks for submitting a pull request! 🎉
Summary
Fixes #3771
Also not described in the issue was that we weren't displaying the correct webhook for gitlab users to add manually since we were passing through
manualinstead ofgitlab- now it actually showsConfigure the following webhook for your repository: https://api.netlify.com/hooks/gitlabas it should (and was in the code, just wasn't being exercised).For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)
