Add repository option to crossplane project init#105
Conversation
📝 WalkthroughWalkthroughThe ChangesProject initialization repository configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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.
There was a problem hiding this comment.
The first component of the repository being an organization name is just a convention, not specified by OCI. That said, it is a very common convention - every public registry I can think of follows it (GHCR, Docker Hub, Upbound marketplace, etc.).
Agreed it's a nice UX to not require repeating the project name. I think making the argument --registry (not technically correct, but less surprising to append to it) or --repository-prefix would make sense. That would allow for a --repository flag to provide the full path as well, if users want the repository name to not match the project name.
|
|
||
| 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.
Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 74-77: The error message in the repository creation block uses
internal wording "cannot build function reference" which is confusing in the
project init command context. Replace this error message with user-facing
wording that clearly indicates the problem is with invalid registry or
repository parameters provided via the --registry and --repository command
flags. Keep the same error wrapping structure but update the message text to be
more actionable and appropriate for the project init command path.
- Around line 61-63: In the Repository field initialization block, the TrimSpace
function is only used for the emptiness check, but the untrimmed value is still
assigned to c.Repository when empty. Persist the trimmed value by assigning the
result of strings.TrimSpace(c.Repository) back to c.Repository before the
fallback check, so that any subsequent validation or usage of the Repository
field at line 74 or elsewhere operates on the normalized value without
accidental leading or trailing whitespace.
- Around line 70-72: The error message returned when the registry path is empty
references the incorrect flag name. In the if block that checks if rp == "",
update the error message to reference the actual flag name `--registry` instead
of `--registryPath` to match the flag definition on line 44 and provide users
with accurate guidance for fixing the issue.
🪄 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: cf7f8fff-566c-4f03-8e2e-bd789ffec85c
📒 Files selected for processing (1)
cmd/crossplane/project/init.go
| if strings.TrimSpace(c.Repository) == "" { | ||
| c.Repository = c.Name | ||
| } |
There was a problem hiding this comment.
Normalize Repository after trimming, before fallback and validation.
Right now TrimSpace is only used for the emptiness check, but Line 74 still consumes the untrimmed value. Could we persist the trimmed value so accidental leading/trailing spaces don’t cause avoidable parse failures?
Suggested patch
- if strings.TrimSpace(c.Repository) == "" {
+ c.Repository = strings.TrimSpace(c.Repository)
+ if c.Repository == "" {
c.Repository = c.Name
}🤖 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 61 - 63, In the Repository field
initialization block, the TrimSpace function is only used for the emptiness
check, but the untrimmed value is still assigned to c.Repository when empty.
Persist the trimmed value by assigning the result of
strings.TrimSpace(c.Repository) back to c.Repository before the fallback check,
so that any subsequent validation or usage of the Repository field at line 74 or
elsewhere operates on the normalized value without accidental leading or
trailing whitespace.
| if rp == "" { | ||
| return errors.New("registry cannot be empty; set --registryPath to an OCI registry prefix like 'xpkg.crossplane.io/my-org'") | ||
| } |
There was a problem hiding this comment.
Use the actual flag name in the empty-registry error message.
Line 71 points users to --registryPath, but the command defines --registry (Line 44). Could we update this so users can recover immediately?
Suggested patch
- return errors.New("registry cannot be empty; set --registryPath to an OCI registry prefix like 'xpkg.crossplane.io/my-org'")
+ return errors.New("registry cannot be empty; set --registry to an OCI repository prefix like 'xpkg.crossplane.io/my-org'")As per coding guidelines, CLI errors in cmd/** should provide actionable user guidance.
🤖 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 70 - 72, The error message
returned when the registry path is empty references the incorrect flag name. In
the if block that checks if rp == "", update the error message to reference the
actual flag name `--registry` instead of `--registryPath` to match the flag
definition on line 44 and provide users with accurate guidance for fixing the
issue.
Source: Coding guidelines
| r, err := name.NewRepository(rp + "/" + c.Repository) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "cannot build function reference from repository \"%s/%s\"", rp, c.Repository) | ||
| } |
There was a problem hiding this comment.
Replace internal “function reference” wording with project init context.
The wrapped message says “cannot build function reference…”, which is confusing in this command path. Please use user-facing wording tied to invalid --registry/--repository input.
Suggested patch
- return errors.Wrapf(err, "cannot build function reference from repository \"%s/%s\"", rp, c.Repository)
+ return errors.Wrapf(err, "invalid repository value %q from --registry/--repository", rp+"/"+c.Repository)Based on learnings, concise error strings are preferred when they clearly state the issue; as per coding guidelines for cmd/**, error text should remain user-facing and actionable.
🤖 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 74 - 77, The error message in
the repository creation block uses internal wording "cannot build function
reference" which is confusing in the project init command context. Replace this
error message with user-facing wording that clearly indicates the problem is
with invalid registry or repository parameters provided via the --registry and
--repository command flags. Keep the same error wrapping structure but update
the message text to be more actionable and appropriate for the project init
command path.
Sources: Coding guidelines, Learnings
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.