-
Notifications
You must be signed in to change notification settings - Fork 50
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
REST: Fix PATCH method #151
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
and thank you so much @isinyaaa
before approving one remaining question: since this was raised by UI team, would you think an update to RobotFramework with a user-story dedicated to using Patch with this PR, on in a subsequent PR, please?
that's surely a good idea, will look into it asap :) |
As that fixed a nil assignment override[1][2] which prevented us from emulating copy constructors[3]. The update also allows us to use generics. [1]: jmattheis/goverter#146 (comment) [2]: jmattheis/goverter#97 [3]: jmattheis/goverter#147 Signed-off-by: Isabella Basso do Amaral <[email protected]> Signed-off-by: Isabella do Amaral <[email protected]>
Signed-off-by: Isabella do Amaral <[email protected]>
Signed-off-by: Isabella do Amaral <[email protected]>
Added a simple test for the use-case. wdyt @tarilabs ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you so much @isinyaaa 👍 🚀
/lgtm
/approve
@@ -130,7 +130,7 @@ bin/golangci-lint: | |||
|
|||
GOVERTER ?= ${PROJECT_BIN}/goverter | |||
bin/goverter: | |||
GOBIN=$(PROJECT_PATH)/bin ${GO} install github.com/jmattheis/goverter/cmd/goverter@v1.1.1 | |||
GOBIN=$(PROJECT_PATH)/bin ${GO} install github.com/jmattheis/goverter/cmd/goverter@v1.4.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bumps goverter 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should eventually look into avoiding specifying the version (it was required for previous work by @lampajr during some ad-interim) ... would you mind @isinyaaa logging an issue/jira to eventually change this to use a latest or some version specified in gomod if it's possible to just consolidate the version please? wdyt? (in a subsequent task of course)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure can look into it. I agree the Makefile is a rather "forgotten" place to specify this, as neither us nor dependabot will keep it up to date.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tarilabs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* update goverter to 1.4.1 As that fixed a nil assignment override[1][2] which prevented us from emulating copy constructors[3]. The update also allows us to use generics. [1]: jmattheis/goverter#146 (comment) [2]: jmattheis/goverter#97 [3]: jmattheis/goverter#147 Signed-off-by: Isabella Basso do Amaral <[email protected]> Signed-off-by: Isabella do Amaral <[email protected]> * simplify converter utils using generics Signed-off-by: Isabella do Amaral <[email protected]> * server: update existing objects on PATCH Signed-off-by: Isabella do Amaral <[email protected]> --------- Signed-off-by: Isabella Basso do Amaral <[email protected]> Signed-off-by: Isabella do Amaral <[email protected]> Signed-off-by: muzhouliu <[email protected]>
Description
Use a converter to reconcile the PATCH body with the existing instance.
Fixes https://issues.redhat.com/browse/RHOAIENG-8534
How Has This Been Tested?
Refer to reproducers in the Jira.
Merge criteria:
cc @tarilabs