Skip to content

first iteration - deepcopy #131

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.

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
merged 3 commits into from
Dec 20, 2022
Merged

Conversation

spolti
Copy link
Member

@spolti spolti commented Nov 25, 2022

Signed-off-by: spolti [email protected]

Fixes #99

Many thanks for submitting your Pull Request ❤️!

What this PR does / why we need it:

Special notes for reviewers:

Additional information (if needed):

@spolti
Copy link
Member Author

spolti commented Nov 25, 2022

@davidesalerno can you please review as well?

@spolti
Copy link
Member Author

spolti commented Nov 25, 2022

It is a work in progress pull request, I've pushed to collect feedback.

@ricardozanini
Copy link
Member

@lsytj0413, I believe the interface{} type (as being the object representation in Go) is not supported by the deepCopy methods IIRC.

@spolti, let's try to keep the generated code in zz_generated(...); this way, we avoid modifications in the non-generated code.

@lsytj0413
Copy link
Collaborator

@lsytj0413, I believe the interface{} type (as being the object representation in Go) is not supported by the deepCopy methods IIRC.

@spolti, let's try to keep the generated code in zz_generated(...); this way, we avoid modifications in the non-generated code.

Yep,in some scenario we can replace interface{} with concrete type(such as IntOrString),but this cannot valid with workflow.States field(the element of States field can be one of ParallelStateSwitchState and so on)

@ricardozanini
Copy link
Member

@lsytj0413, I believe the interface{} type (as being the object representation in Go) is not supported by the deepCopy methods IIRC.
@spolti, let's try to keep the generated code in zz_generated(...); this way, we avoid modifications in the non-generated code.

Yep,in some scenario we can replace interface{} with concrete type(such as IntOrString),but this cannot valid with workflow.States field(the element of States field can be one of ParallelStateSwitchState and so on)

I believe that we can work that out with well-defined types.

I'll wait for this one to be done to release the new version.

@spolti
Copy link
Member Author

spolti commented Dec 1, 2022

I am currently trying to add the new Type as suggested, IntOrObject (based on the same approach from k8s apimachinery used for IntOrString).
Let's see how that goes.

@lsytj0413, I believe the interface{} type (as being the object representation in Go) is not supported by the deepCopy methods IIRC.
@spolti, let's try to keep the generated code in zz_generated(...); this way, we avoid modifications in the non-generated code.

Yep,in some scenario we can replace interface{} with concrete type(such as IntOrString),but this cannot valid with workflow.States field(the element of States field can be one of ParallelStateSwitchState and so on)

I believe that we can work that out with well-defined types.

I'll wait for this one to be done to release the new version.

@spolti spolti force-pushed the deepCopy branch 2 times, most recently from 095c2c1 to 50d85e1 Compare December 8, 2022 20:09
@spolti
Copy link
Member Author

spolti commented Dec 8, 2022

Hi guys, please refer to my last commit, I got something "workable", I've added the IntOrObject custom object that allow us to have basically a object of any kind (for now, int, string and interface{}).

This first proposal exemplifies two possible ways to implement it:

First, have Integer, String and Custom fields, that we will have something like:

"time":model.IntOrObject{IntVal:(*int)(0x1400027bdb8), StrVal:(*int)(nil), ObjVal:model.Object(nil)}})

and the second option will be having only the ObjVal field, producing something like:

 "time":model.IntOrObject{ObjVal:model.MyInt{Value:48}}})

With both approach we can parse the following (tested only with arguments for now):

  - name: CheckCreditCallback
    type: callback
    action:
      functionRef:
        refName: callCreditCheckMicroservice
        arguments:
          customer: "${ .customer }"
          argsObj: {
                   "name" : "hi",
                   "age": 10
          }
          time: 48

Can be check in the failing test, the output would be:

expected: map[string]string(map[string]string{"customer":"${ .customer }"})
actual  : map[string]model.IntOrObject(map[string]model.IntOrObject{"customer":model.IntOrObject{IntVal:(*int)(nil), ObjVal:model.MyString{Value:"${ .customer }"}}, "argsObj":model.IntOrObject{IntVal:(*int)(nil), ObjVal:model.custom{Object:map[string]interface {}{"age":10, "name":"hi"}}}, "time":model.IntOrObject{IntVal:(*int)(nil), ObjVal:model.MyInt{Value:48}}})

@ricardozanini
Copy link
Member

Seems fine, but can you take a look at the CI faiulres?

@spolti
Copy link
Member Author

spolti commented Dec 9, 2022

CI is failing on purpose, so you guys can take a look on the diference between the previous and the proposed version of the custom object.

@spolti
Copy link
Member Author

spolti commented Dec 9, 2022

about sonatype-lift, it is complaining about the deepCopy generated file, asked it to be ignored already, hopefully it will be green once it runs again.

@ricardozanini
Copy link
Member

CI is failing on purpose, so you guys can take a look on the diference between the previous and the proposed version of the custom object.

let's wait for @lsytj0413 feedback. We can set a milestone next Monday and go ahead with the changes to the other structures.

@spolti
Copy link
Member Author

spolti commented Dec 12, 2022

@ricardozanini about the option, what option do you think better?
Planning to finish the changes tomorrow.

@spolti
Copy link
Member Author

spolti commented Dec 13, 2022

Hi guys, please refer to my last commit, I got something "workable", I've added the IntOrObject custom object that allow us to have basically a object of any kind (for now, int, string and interface{}).

This first proposal exemplifies two possible ways to implement it:

First, have Integer, String and Custom fields, that we will have something like:

"time":model.IntOrObject{IntVal:(*int)(0x1400027bdb8), StrVal:(*int)(nil), ObjVal:model.Object(nil)}})

and the second option will be having only the ObjVal field, producing something like:

 "time":model.IntOrObject{ObjVal:model.MyInt{Value:48}}})

With both approach we can parse the following (tested only with arguments for now):

  - name: CheckCreditCallback
    type: callback
    action:
      functionRef:
        refName: callCreditCheckMicroservice
        arguments:
          customer: "${ .customer }"
          argsObj: {
                   "name" : "hi",
                   "age": 10
          }
          time: 48

Can be check in the failing test, the output would be:

expected: map[string]string(map[string]string{"customer":"${ .customer }"})
actual  : map[string]model.IntOrObject(map[string]model.IntOrObject{"customer":model.IntOrObject{IntVal:(*int)(nil), ObjVal:model.MyString{Value:"${ .customer }"}}, "argsObj":model.IntOrObject{IntVal:(*int)(nil), ObjVal:model.custom{Object:map[string]interface {}{"age":10, "name":"hi"}}}, "time":model.IntOrObject{IntVal:(*int)(nil), ObjVal:model.MyInt{Value:48}}})

@lsytj0413 before review, please see this comment and share your thoughts on the best option based on the outputs.
nothing is final yet it is just a draft on how it would be.

@spolti spolti force-pushed the deepCopy branch 2 times, most recently from 7ec2619 to 704e036 Compare December 13, 2022 21:10
@spolti spolti force-pushed the deepCopy branch 11 times, most recently from 4e5a2ee to 3777a19 Compare December 14, 2022 19:21
@ricardozanini
Copy link
Member

@ricardozanini about the option, what option do you think better? Planning to finish the changes tomorrow.

Go ahead and implementing a specific type for each use case.

@spolti spolti force-pushed the deepCopy branch 9 times, most recently from bd9c4ac to 67b3a72 Compare December 15, 2022 14:15
Signed-off-by: spolti <[email protected]>
Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

It's fine if you want to provide the latest comment from my review in a new PR.

@ricardozanini
Copy link
Member

@lsytj0413 wanna take a final look? I'm willing to merge this one to finally release 0.8.

@ricardozanini ricardozanini merged commit a073b0e into serverlessworkflow:main Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate DeepCopy methods
3 participants