Revert back to using cross-resource reference utilities from crossplane-runtime #106
Conversation
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
sergenyalcin
changed the title
ResolvedValue as nil in reference resolvers and fix *float64 resolver generationResolvedValue as nil in reference resolvers and fix *float64 resolver generation
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
|
for the second issue xref: #102 ? |
internal/method/resolver_test.go
Outdated
| if v := rsp.ResolvedValue; v != "" { | ||
| mg.Spec.ForProvider.SecurityGroupID = ptr.To(v) | ||
| } else { | ||
| mg.Spec.ForProvider.SecurityGroupID = nil | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. 😄
There was a problem hiding this comment.
Reverted this crossplane/crossplane-runtime#780 and reused in here.
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
|
Can a fix be merged for this? |
ulucinar
left a comment
ulucinar
left a comment
There was a problem hiding this comment.
Hi @sergenyalcin,
This PR looks good to me. There are a couple of points worth mentioning:
- In the package
pkg/convert, we have a bunch of deprecated conversion functions in favor of those in k8s.io/utils/ptr. Because their behaviors differ, we may consider to remove the deprecation notices also. But I propose we do so in a follow-up PR. - Looking at the comment in that package. my understanding is that it has been added as the package to import for the APIs. If this was the intent, we are still using the re-added utility functions from crossplane-runtime. Because this was already the old behavior, I think this is fine for this PR. We may follow-up if this is really the intent.
- Some "new" providers may have used the recent versions of crossplane-tools and may be depending on the behavior of the k8s.io/utils/ptr package. And we are reverting back the behavior.
| ClientImport: ClientAlias, | ||
| ConvertImport: ConvertAlias, | ||
| ReferenceImport: ReferenceAlias, | ||
| PtrAlias: PtrImport, |
There was a problem hiding this comment.
Interesting. Shouldn't this have been PtrImport: PtrAlias (key & value reversed) in the first place? Looks like the function expects a map where keys are imports, not aliases?
ulucinar
changed the title
ResolvedValue as nil in reference resolvers and fix *float64 resolver generation5 participants
Description of your changes
Fixes #103
This PR uses the reintroduced crossplane-runtime helper functions: crossplane/crossplane-runtime#833.
Since the behavior change in the
ptr.Deref, we considered reusing them. Details are in the issue.I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested
Tested in the provider-upjet-gcp and provider-upjet-azuread locally.