Skip to content

Revert back to using cross-resource reference utilities from crossplane-runtime #106

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ulucinar merged 5 commits into from
Jun 3, 2025
+47 −37
Merged

Revert back to using cross-resource reference utilities from crossplane-runtime #106

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e9a9112
Add a nil check for reference resolvers
sergenyalcin May 2, 2025
4383b0a
Fix the *float64 reference generation
sergenyalcin May 5, 2025
5da36d8
Add new case to unit-tests for *float64 type reference generation
sergenyalcin May 5, 2025
1106a26
Reuse the crossplane-runtime helpers
sergenyalcin May 9, 2025
3c38a1e
Change the multi resolvers
sergenyalcin May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions internal/method/resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func NewResolveReferences(traverser *xptypes.Traverser, receiver, clientPath, re
resolverCalls[i] = encapsulate(0, multiResolutionCall(ref, referencePkgPath, convertPkgPath), ref.GoValueFieldPath...).Line()
} else {
hasSingleResolution = true
resolverCalls[i] = encapsulate(0, singleResolutionCall(ref, referencePkgPath, ptrPkgPath), ref.GoValueFieldPath...).Line()
resolverCalls[i] = encapsulate(0, singleResolutionCall(ref, referencePkgPath, ptrPkgPath, convertPkgPath), ref.GoValueFieldPath...).Line()
}
}
var initStatements jen.Statement
Expand Down Expand Up @@ -113,7 +113,7 @@ func encapsulate(index int, callFn resolutionCallFn, fields ...string) *jen.Stat
}
}

func singleResolutionCall(ref Reference, referencePkgPath string, ptrPkgPath string) resolutionCallFn {
func singleResolutionCall(ref Reference, referencePkgPath string, ptrPkgPath string, convertPkgPath string) resolutionCallFn {
return func(fields ...string) *jen.Statement {
prefixPath := jen.Id(fields[0])
for i := 1; i < len(fields)-1; i++ {
Expand All @@ -124,15 +124,21 @@ func singleResolutionCall(ref Reference, referencePkgPath string, ptrPkgPath str
selectorFieldPath := prefixPath.Clone().Dot(ref.GoSelectorFieldName)

setResolvedValue := currentValuePath.Clone().Op("=").Id("rsp").Dot("ResolvedValue")
pkgPath := ptrPkgPath
toPointerFunction := "To"
fromPointerFunction := "Deref"
if ref.IsFloatPointer {
toPointerFunction = "ToFloatPtrValue"
fromPointerFunction = "FromFloatPtrValue"
}
if ref.IsPointer {
setResolvedValue = currentValuePath.Clone().Op("=").Qual(ptrPkgPath, toPointerFunction).Call(jen.Id("rsp").Dot("ResolvedValue"))
currentValuePath = jen.Qual(ptrPkgPath, fromPointerFunction).Call(currentValuePath, jen.Op(`""`))
if ref.IsFloatPointer {
toPointerFunction = "ToFloatPtrValue"
fromPointerFunction = "FromFloatPtrValue"
pkgPath = convertPkgPath
currentValuePath = jen.Qual(pkgPath, fromPointerFunction).Call(currentValuePath)
} else {
currentValuePath = jen.Qual(pkgPath, fromPointerFunction).Call(currentValuePath, jen.Op(`""`))
}
setResolvedValue = jen.If(jen.Id("v").Op(":=").Id("rsp").Dot("ResolvedValue")).Op(";").Id("v").Op("!=").Lit("").Block(
jen.Add(prefixPath.Clone().Dot(fields[len(fields)-1]).Clone().Op("=").Qual(pkgPath, toPointerFunction).Call(jen.Id("v")))).Else().Block(
jen.Add(prefixPath.Clone().Dot(fields[len(fields)-1]).Clone().Op("=").Nil()))
}
return &jen.Statement{
jen.List(jen.Id("rsp"), jen.Err()).Op("=").Id("r").Dot("Resolve").Call(
Expand Down
60 changes: 54 additions & 6 deletions internal/method/resolver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ type ModelParameters struct {
// +crossplane:generate:reference:type=golang.org/fake/v1alpha1.Configuration
// +crossplane:generate:reference:extractor=golang.org/fake/v1alpha1.Configuration()
CustomConfiguration *Configuration

// +crossplane:generate:reference:type=github.com/crossplane/provider-aws/apis/identity/v1beta1.IAM
// +crossplane:generate:reference:extractor=Count()
Count *float64
}

type NetworkSpec struct {
Expand Down Expand Up @@ -150,7 +154,11 @@ func (mg *Model) ResolveReferences(ctx context.Context, c client.Reader) error {
if err != nil {
return errors.Wrap(err, "mg.Spec.ForProvider.SecurityGroupID")
}
mg.Spec.ForProvider.SecurityGroupID = ptr.To(rsp.ResolvedValue)
if v := rsp.ResolvedValue; v != "" {
mg.Spec.ForProvider.SecurityGroupID = ptr.To(v)
} else {
mg.Spec.ForProvider.SecurityGroupID = nil
}
Copy link
Member

@negz negz May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to avoid all these if else clauses. Could we instead reintroduce ToPtrValue?

I suggested we remove it because I thought it did the same thing as ptr.To. Given I was wrong, I'd rather keep it than inline its logic here.

Copy link
Member Author

@sergenyalcin sergenyalcin May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @negz for your review.

That would be better, too. Actually, I would prefer to keep the old function usage rather than adding this complexity to reference resolvers. However, I didn't want to bring this up again, as these functions have been removed there, and I don't think they are intended to be used. Instead, this is how I solved it in its current form.

If we are going to reuse this ToPtrValue, I'm okay with that too, and that would be even better. It is safer and makes more sense than carrying this logic to resolvers.

In this context, would you recommend reintroducing these functions in crossplane-runtime or in the crossplane-tools repo?

Copy link
Member

@negz negz May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to re-add them to runtime. I don't think we use Crossplane-tools as a library anywhere, right? Instead it's just CLI utils that generate code.

At this point it may make sense to just revert crossplane/crossplane-runtime#780. It sounds like the premise of that PR was flawed: I thought our library of pointer helpers did the same thing as the k8s.io/ptr package, but they didn't. So there's value in having our own package.

Disclaimer: I'm stretched extremely thin right now and don't have much time to double check my understanding. My guidance is:

  • I'd prefer us to not have code in crossplane-runtime if there's an upstream alternative that does the same thing
  • It's fine to have code in crossplane-runtime that does something useful to us

If it seems to you that the pointer utils we deleted satisfy the above constraints, please re-add them. 😄

Copy link
Member Author

@sergenyalcin sergenyalcin May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted this crossplane/crossplane-runtime#780 and reused in here.

mg.Spec.ForProvider.SecurityGroupIDRef = rsp.ResolvedReference

rsp, err = r.Resolve(ctx, reference.ResolutionRequest{
Expand All @@ -166,7 +174,11 @@ func (mg *Model) ResolveReferences(ctx context.Context, c client.Reader) error {
if err != nil {
return errors.Wrap(err, "mg.Spec.ForProvider.IAMRoleARN")
}
mg.Spec.ForProvider.IAMRoleARN = ptr.To(rsp.ResolvedValue)
if v := rsp.ResolvedValue; v != "" {
mg.Spec.ForProvider.IAMRoleARN = ptr.To(v)
} else {
mg.Spec.ForProvider.IAMRoleARN = nil
}
mg.Spec.ForProvider.IAMRoleARNRef = rsp.ResolvedReference

rsp, err = r.Resolve(ctx, reference.ResolutionRequest{
Expand All @@ -182,7 +194,11 @@ func (mg *Model) ResolveReferences(ctx context.Context, c client.Reader) error {
if err != nil {
return errors.Wrap(err, "mg.Spec.ForProvider.NestedTargetWithPath")
}
mg.Spec.ForProvider.NestedTargetWithPath = ptr.To(rsp.ResolvedValue)
if v := rsp.ResolvedValue; v != "" {
mg.Spec.ForProvider.NestedTargetWithPath = ptr.To(v)
} else {
mg.Spec.ForProvider.NestedTargetWithPath = nil
}
mg.Spec.ForProvider.NestedTargetWithPathRef = rsp.ResolvedReference

rsp, err = r.Resolve(ctx, reference.ResolutionRequest{
Expand All @@ -198,7 +214,11 @@ func (mg *Model) ResolveReferences(ctx context.Context, c client.Reader) error {
if err != nil {
return errors.Wrap(err, "mg.Spec.ForProvider.NestedTargetNoPath")
}
mg.Spec.ForProvider.NestedTargetNoPath = ptr.To(rsp.ResolvedValue)
if v := rsp.ResolvedValue; v != "" {
mg.Spec.ForProvider.NestedTargetNoPath = ptr.To(v)
} else {
mg.Spec.ForProvider.NestedTargetNoPath = nil
}
mg.Spec.ForProvider.NestedTargetNoPathRef = rsp.ResolvedReference

rsp, err = r.Resolve(ctx, reference.ResolutionRequest{
Expand All @@ -214,7 +234,11 @@ func (mg *Model) ResolveReferences(ctx context.Context, c client.Reader) error {
if err != nil {
return errors.Wrap(err, "mg.Spec.ForProvider.NoArgNoPath")
}
mg.Spec.ForProvider.NoArgNoPath = ptr.To(rsp.ResolvedValue)
if v := rsp.ResolvedValue; v != "" {
mg.Spec.ForProvider.NoArgNoPath = ptr.To(v)
} else {
mg.Spec.ForProvider.NoArgNoPath = nil
}
mg.Spec.ForProvider.NoArgNoPathRef = rsp.ResolvedReference

if mg.Spec.ForProvider.Network != nil {
Expand Down Expand Up @@ -298,9 +322,33 @@ func (mg *Model) ResolveReferences(ctx context.Context, c client.Reader) error {
if err != nil {
return errors.Wrap(err, "mg.Spec.ForProvider.CustomConfiguration")
}
mg.Spec.ForProvider.CustomConfiguration = ptr.To(rsp.ResolvedValue)
if v := rsp.ResolvedValue; v != "" {
mg.Spec.ForProvider.CustomConfiguration = ptr.To(v)
} else {
mg.Spec.ForProvider.CustomConfiguration = nil
}
mg.Spec.ForProvider.CustomConfigurationRef = rsp.ResolvedReference

rsp, err = r.Resolve(ctx, reference.ResolutionRequest{
CurrentValue: convert.FromFloatPtrValue(mg.Spec.ForProvider.Count),
Extract: Count(),
Reference: mg.Spec.ForProvider.CountRef,
Selector: mg.Spec.ForProvider.CountSelector,
To: reference.To{
List: &v1beta1.IAMList{},
Managed: &v1beta1.IAM{},
},
})
if err != nil {
return errors.Wrap(err, "mg.Spec.ForProvider.Count")
}
if v := rsp.ResolvedValue; v != "" {
mg.Spec.ForProvider.Count = convert.ToFloatPtrValue(v)
} else {
mg.Spec.ForProvider.Count = nil
}
mg.Spec.ForProvider.CountRef = rsp.ResolvedReference

return nil
}
`
Expand Down
Loading