Skip to content
This repository has been archived by the owner on Jun 21, 2022. It is now read-only.

PMM-9377 add compatibility with updated grpc-gateway v2 #1005

Closed
wants to merge 5 commits into from
Closed

PMM-9377 add compatibility with updated grpc-gateway v2 #1005

wants to merge 5 commits into from

Conversation

idoqo
Copy link
Contributor

@idoqo idoqo commented Feb 25, 2022 •
edited
Loading

@codecov
Copy link

codecov bot commented Feb 25, 2022 •
edited
Loading

Codecov Report

Merging #1005 (2a41b39) into main (670fc56) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1005      +/-   ##
==========================================
- Coverage   49.77%   49.76%   -0.02%     
==========================================
  Files         173      173              
  Lines       20404    20409       +5     
==========================================
  Hits        10157    10157              
- Misses       9114     9119       +5     
  Partials     1133     1133
Flag Coverage Δ
all 49.40% <0.00%> (-0.02%) ⬇️
cover 49.69% <0.00%> (-0.02%) ⬇️
crosscover 49.76% <0.00%> (-0.02%) ⬇️
update 12.61% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
main.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 670fc56...2a41b39. Read the comment docs.

@idoqo idoqo marked this pull request as ready for review February 28, 2022 04:47
@idoqo idoqo requested a review from BupycHuk as a code owner February 28, 2022 04:47
@idoqo idoqo mentioned this pull request Feb 28, 2022
Closed
2 tasks
@idoqo idoqo changed the title PMM-9377 use grpc-gateway@v2 branch PMM-9377 add compatibility with updated grpc-gateway v2 Feb 28, 2022
@idoqo idoqo mentioned this pull request Feb 28, 2022
Closed
3 tasks
@JNKPercona JNKPercona mentioned this pull request Mar 9, 2022
Closed
3 tasks
@idoqo idoqo mentioned this pull request Mar 9, 2022
Closed
1 task
@idoqo idoqo force-pushed the PMM-9377-update-grpc-gateway branch 3 times, most recently from 1985d98 to 4898d77 Compare March 14, 2022 12:14
BupycHuk
BupycHuk reviewed Mar 14, 2022
View reviewed changes
api-tests/helpers.go
Comment on lines +73 to +74
errorField := payload.Elem().FieldByName("Message")
require.True(t, errorField.IsValid(), "Wrong response structure. There is no field Message in Payload.")
Copy link
Member

Choose a reason for hiding this comment

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

we should keep the same structure for backward compatibility

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem though is that the Error field is no longer included in the struct as a result of this PR: Remove "error" field from errors.

api-tests/server/settings_test.go
Lr: "2m",
Lr: "120s",
Copy link
Member

Choose a reason for hiding this comment

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

why did you change these values? don't we support them anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - v2 started using protojson for marshaling/unmarshalling JSON, and it appears it now strictly uses the protobuf JSON mapping described here.

I checked a few dashboard settings and they seem to already be using only s prefix, but something else somewhere probably depends on this.

@idoqo idoqo mentioned this pull request Mar 17, 2022
Closed
3 tasks
@idoqo idoqo force-pushed the PMM-9377-update-grpc-gateway branch 2 times, most recently from 6a6f6f4 to 8c65d66 Compare March 21, 2022 07:27
@idoqo idoqo force-pushed the PMM-9377-update-grpc-gateway branch from 8c65d66 to 2a41b39 Compare March 28, 2022 08:06
@BupycHuk BupycHuk self-requested a review March 28, 2022 10:30
@idoqo idoqo closed this May 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@ademidoff ademidoff ademidoff approved these changes

@BupycHuk BupycHuk BupycHuk approved these changes

@palash25 palash25 Awaiting requested review from palash25

@artemgavrilov artemgavrilov Awaiting requested review from artemgavrilov

@PavelKhripkov PavelKhripkov Awaiting requested review from PavelKhripkov

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@idoqo @ademidoff @BupycHuk