feat: add AddReference method to References#415
feat: add AddReference method to References#415yordis wants to merge 1 commit intocrossplane:mainfrom
Conversation
yordis
force-pushed
the
feat-improve-sdk-reference
branch
2 times, most recently
from
30b0582 to
3da3f28
Compare
yordis
requested review from
erhancagirici,
mergenci,
sergenyalcin and
ulucinar
as code owners
|
A few things from this,
|
yordis
force-pushed
the
feat-improve-sdk-reference
branch
2 times, most recently
from
67ef84e to
797d5c9
Compare
| // in another object. | ||
| func (r References) AddReference(fieldPath string, ref Reference) error { | ||
| if _, ok := r[fieldPath]; ok { | ||
| return ErrReferenceAlreadyExists |
There was a problem hiding this comment.
I don't love returning an error in this situation. My instinct is that the action of setting a reference should be idempotent; as you've implemented it all times it's invoked but the first will fail with an error.
Maybe it's ok because it would happen at generation time, although I don't know if we could guarantee that.
There was a problem hiding this comment.
I have no strong opinions; either way it is fine by me
There was a problem hiding this comment.
Hey @mbbush, what would be the resolution here? I am not sure if you requested some changes or not
|
|
1 similar comment
|
|
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
feat-improve-sdk-reference
branch
from
797d5c9 to
97604aa
Compare
📝 WalkthroughWalkthroughIntroduces a new sentinel error Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 1
🧹 Nitpick comments (1)
pkg/config/resource_test.go (1)
300-329: ⚡ Quick winPlease align this new test with the repo’s table-driven + PascalCase convention.
Could we rename
TestReferences_AddReferencetoTestReferencesAddReferenceand convert the two scenarios into a small table (args/want) to match the file and repo standard? This should also make future AddReference cases easier to extend.As per coding guidelines "
**/*_test.go: Enforce table-driven test structure: PascalCase test names (no underscores), args/want pattern..."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/resource_test.go` around lines 300 - 329, Rename the test from TestReferences_AddReference to TestReferencesAddReference and rewrite it as a table-driven test using args/want style: create a test table with two cases (e.g., "AddingReference" and "AddingDuplicateReference") each containing the initial Resource/References state and the Reference to add, expected error (nil or ErrReferenceAlreadyExists) and expected length; iterate cases calling References.AddReference on Resource (use Resource and References.AddReference symbols), assert error matches expected and length of r.References matches want; keep PascalCase test name and args/want variables to conform to repo conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/config/resource_test.go`:
- Around line 309-311: The failure message when checking len(r.References) is
misleading because it prints the unrelated err; update the t.Fatalf in the test
that calls AddReference so it reports the actual length and contents of
r.References instead of "AddReference() got error". Concretely, change the
t.Fatalf that triggers on len(r.References) != 1 to something like: include
expected vs actual count and the r.References value (e.g., "AddReference():
expected 1 reference, got %d: %#v", len(r.References), r.References), and remove
the err value from that message.
---
Nitpick comments:
In `@pkg/config/resource_test.go`:
- Around line 300-329: Rename the test from TestReferences_AddReference to
TestReferencesAddReference and rewrite it as a table-driven test using args/want
style: create a test table with two cases (e.g., "AddingReference" and
"AddingDuplicateReference") each containing the initial Resource/References
state and the Reference to add, expected error (nil or
ErrReferenceAlreadyExists) and expected length; iterate cases calling
References.AddReference on Resource (use Resource and References.AddReference
symbols), assert error matches expected and length of r.References matches want;
keep PascalCase test name and args/want variables to conform to repo
conventions.
🪄 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: ac6358e0-043b-46ec-af7c-ef2e820654a5
📒 Files selected for processing (2)
pkg/config/resource.gopkg/config/resource_test.go
| if len(r.References) != 1 { | ||
| t.Fatalf("AddReference() got error: %v", err) | ||
| } |
There was a problem hiding this comment.
Line 310 has a misleading failure message.
When len(r.References) != 1, the test reports AddReference() got error, which can print <nil> and obscure the actual failure. Please report the map length/value mismatch instead.
Suggested fix
- if len(r.References) != 1 {
- t.Fatalf("AddReference() got error: %v", err)
- }
+ if len(r.References) != 1 {
+ t.Fatalf("AddReference() expected 1 reference, got %d", len(r.References))
+ }📝 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.
| if len(r.References) != 1 { | |
| t.Fatalf("AddReference() got error: %v", err) | |
| } | |
| if len(r.References) != 1 { | |
| t.Fatalf("AddReference() expected 1 reference, got %d", len(r.References)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/config/resource_test.go` around lines 309 - 311, The failure message when
checking len(r.References) is misleading because it prints the unrelated err;
update the t.Fatalf in the test that calls AddReference so it reports the actual
length and contents of r.References instead of "AddReference() got error".
Concretely, change the t.Fatalf that triggers on len(r.References) != 1 to
something like: include expected vs actual count and the r.References value
(e.g., "AddReference(): expected 1 reference, got %d: %#v", len(r.References),
r.References), and remove the err value from that message.
3 participants
Description of your changes
@haarchri was helping at crossplane-contrib/provider-upjet-digitalocean#41 (thank you), and during the code review, I noticed a oversee where he replaced the existing
r.Referencesas follow:I am glad that I saw the diff, but honestly, this is a straightforward thing to miss when they are so much code-generated files noise, or people tend to ignore those.
As @haarchri explained, "That's why I prefer this, for example, https://github.com/crossplane-contrib/provider-upjet-aws/blob/main/config/elbv2/config.go#L13," which is a really common scenario.
I never considered using the existing style. I didn't think much about it. He didn’t try to break things. Still, we ran into the issue. Since we are talking about "style" here, it is really difficult to be objective and define who is more proper.
So, the fix here is to expose an SDK API that avoids such a mistake.
I am not proposing adding getters and setters everywhere either, just reacting to the situation and figuring out how to mitigate the issues in the future; as I usually say, "let the code teach you what to do, not the other way around."
I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested