Add repository option to crossplane project init#105
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ChangesProject initialization repository configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/crossplane/project/init.go`:
- Around line 77-78: Normalize and validate the repository base (c.Repository)
before composing spec.repository: ensure c.Repository is non-empty and trim any
trailing slashes or whitespace, then join it with c.Name using a safe join
(e.g., path.Join or by appending "/" then name) instead of raw "%s/%s"
concatenation so you never produce double slashes; update the code that formats
spec.repository (the spot using c.Repository and c.Name) to use the normalized
value and return or surface a validation error if repository is empty/invalid.
- Line 42: The Directory field in the project init command is currently declared
as a positional arg (arg:"") but also has a short flag, so --directory/-d won't
parse; change Directory in cmd/crossplane/project/init.go from a positional to a
real flag by removing arg:"" and replacing it with a name tag and the existing
short/help/type tags (e.g. name:"directory" short:"d" help:"Directory to
initialize..." type:"path"), and if you instead prefer a second positional arg
update the command help/examples in cmd/crossplane/project/help/init.md to show
the positional usage; locate the Directory field in the relevant struct to apply
the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb516f46-3c47-48a4-a6a1-4696dba3d528
📒 Files selected for processing (1)
cmd/crossplane/project/init.go
Make the directory name truly optional. Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/crossplane/project/init.go (1)
65-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWhitespace-only repository values can pass validation and generate invalid
spec.repository.Could you confirm whether
--repository " "should be rejected? On Line 65, only trailing/is trimmed; on Line 66 the emptiness check can be bypassed by whitespace, which then propagates to Line 81 and fails later in downstream repository parsing.Suggested minimal fix
- repo := strings.TrimRight(c.Repository, "/") + repo := strings.TrimRight(strings.TrimSpace(c.Repository), "/") if repo == "" { return errors.New("repository cannot be empty; set --repository to an OCI repository prefix like 'xpkg.crossplane.io/my-org'") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/project/init.go` around lines 65 - 68, The current validation trims only trailing slashes and allows all-whitespace repository values to pass; update the handling of c.Repository by applying strings.TrimSpace (and still TrimRight for slashes if needed) to produce repo, then validate repo == "" after trimming so that inputs like "--repository \" \"" are rejected; ensure the rest of the function (where repo is used to set spec.repository) uses this trimmed repo variable (referencing c.Repository, repo) so downstream parsing no longer sees whitespace-only values.
🧹 Nitpick comments (1)
cmd/crossplane/project/init.go (1)
50-83: ⚡ Quick winPlease add focused tests for repository normalization and optional directory behavior.
Thanks for the feature update — would you add table-driven coverage for: default repository, trailing slash trimming, whitespace-only rejection, and omitted directory defaulting to project name? This should lock in the intended CLI UX and prevent regressions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/project/init.go` around lines 50 - 83, Add table-driven unit tests for initCmd.Run to validate repository normalization and directory defaulting: create test cases for (1) default repository value (empty repo should error with the same message as currently returned for repo == ""), (2) repository with trailing slash gets normalized (strings.TrimRight) and appears in generated content, (3) whitespace-only repository is rejected, and (4) omitted Directory results in c.Directory being set to c.Name and used to create projectFileName content; instantiate initCmd with appropriate fields, call Run with a no-op SpinnerPrinter or wrap the inner logic (refer to initCmd.Run, c.Repository, c.Directory, checkTargetDirectory, and projectFileName) and assert expected errors or generated file/content behavior for each table row.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@cmd/crossplane/project/init.go`:
- Around line 65-68: The current validation trims only trailing slashes and
allows all-whitespace repository values to pass; update the handling of
c.Repository by applying strings.TrimSpace (and still TrimRight for slashes if
needed) to produce repo, then validate repo == "" after trimming so that inputs
like "--repository \" \"" are rejected; ensure the rest of the function (where
repo is used to set spec.repository) uses this trimmed repo variable
(referencing c.Repository, repo) so downstream parsing no longer sees
whitespace-only values.
---
Nitpick comments:
In `@cmd/crossplane/project/init.go`:
- Around line 50-83: Add table-driven unit tests for initCmd.Run to validate
repository normalization and directory defaulting: create test cases for (1)
default repository value (empty repo should error with the same message as
currently returned for repo == ""), (2) repository with trailing slash gets
normalized (strings.TrimRight) and appears in generated content, (3)
whitespace-only repository is rejected, and (4) omitted Directory results in
c.Directory being set to c.Name and used to create projectFileName content;
instantiate initCmd with appropriate fields, call Run with a no-op
SpinnerPrinter or wrap the inner logic (refer to initCmd.Run, c.Repository,
c.Directory, checkTargetDirectory, and projectFileName) and assert expected
errors or generated file/content behavior for each table row.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 93e17c21-4531-4195-950c-bdee283d8b13
📒 Files selected for processing (1)
cmd/crossplane/project/init.go
Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
|
Docs PR crossplane/docs#1108 |
| repository: %s/%s | ||
| `, c.Name, repo, c.Name) |
There was a problem hiding this comment.
I'd prefer to just use repo when it's specified, rather than appending the project name. OCI defines two things: registries and repositories; the in-between of a partially-specified repository isn't really a concept.
There was a problem hiding this comment.
Don't we really have three parts - the registry, the org and the repository? Maybe the option should be registryPath or similar to include both the registry and the org? I'd like to be able to default the repository name to the project name rather than have to specify it again in the registry path.
|
|
||
| repo := strings.TrimRight(strings.TrimSpace(c.Repository), "/") | ||
| if repo == "" { | ||
| return errors.New("repository cannot be empty; set --repository to an OCI repository prefix like 'xpkg.crossplane.io/my-org'") |
There was a problem hiding this comment.
Might be worth validating that repo is a valid, fully-qualified repository by calling name.NewRepository.
2 participants
Description of your changes
Added the
--repositoryoption tocrossplane project initto override the defaultexample.com/my-orgvalues.Made the directory name truly optional.
I have:
./nix.sh flake checkto ensure this PR is ready for review.- [ ] Added or updated unit tests.backport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.