Skip to content
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

fix json unmarshal error when list plguins #888

Merged
merged 1 commit into from Mar 2, 2022

Conversation

nayihz
Copy link
Contributor

@nayihz nayihz commented Feb 25, 2022

Type of change:

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

What this PR does / why we need it:

bug fix #883
the response body is JSON with unknown field names but not a string slice.

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@tao12345666333 tao12345666333 added the bug Something isn't working label Feb 25, 2022
@tao12345666333 tao12345666333 added this to In progress in v1.5 Planning via automation Feb 25, 2022
@nayihz nayihz marked this pull request as draft February 25, 2022 12:23
@codecov-commenter
Copy link

codecov-commenter commented Feb 26, 2022

Codecov Report

Merging #888 (4c0ed87) into master (e2475cf) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 4c0ed87 differs from pull request most recent head 1740faf. Consider uploading reports for the commit 1740faf to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #888      +/-   ##
==========================================
+ Coverage   32.01%   32.04%   +0.02%     
==========================================
  Files          70       70              
  Lines        7749     7752       +3     
==========================================
+ Hits         2481     2484       +3     
  Misses       4993     4993              
  Partials      275      275              
Impacted Files Coverage Δ
pkg/apisix/cluster.go 33.03% <100.00%> (+0.45%) ⬆️

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 e2475cf...1740faf. Read the comment docs.

@nayihz nayihz marked this pull request as ready for review February 28, 2022 04:29
@tao12345666333
Copy link
Member

Thanks! It's on my list

Copy link
Contributor

@tokers tokers left a comment

Choose a reason for hiding this comment

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

@cmssczy Thanks for your PR, could you give us an example about the response? Fair to say, I didn't realize the difference.

pkg/apisix/cluster.go Outdated Show resolved Hide resolved
@nayihz
Copy link
Contributor Author

nayihz commented Mar 1, 2022

could you give us an example about the response? Fair to say, I didn't realize the difference.

@tokers
The response body json format is like:

{
	"serverless-pre-function": {
		"version": 0.1,
		"priority": 10000
	},
	"echo": {
		"version": 0.1,
		"priority": 412
	}
}

It is a json struct with unknown field names but not a string slice. So I think we should use a map instead of slice.

Copy link
Member

@tao12345666333 tao12345666333 left a comment

Choose a reason for hiding this comment

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

LGTM

@lingsamuel
Copy link
Member

We need a mechanism to ensure that the APISIX interface we are using is not outdated.

v1.5 Planning automation moved this from In progress to Reviewer approved Mar 2, 2022
@tao12345666333
Copy link
Member

We need a mechanism to ensure that the APISIX interface we are using is not outdated.

yep. we may need to convert directly from the upstream schema.

now let's move forward.

@tao12345666333 tao12345666333 merged commit cc9b6be into apache:master Mar 2, 2022
v1.5 Planning automation moved this from Reviewer approved to Done Mar 2, 2022
@nayihz nayihz deleted the fix-json-error branch March 2, 2022 03:33
tao12345666333 pushed a commit to tao12345666333/ingress-controller that referenced this pull request Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

bug: failed to list plugins' names
5 participants