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: missing etcd init_dir and unable to list resource #10569

Merged
merged 3 commits into from
Nov 30, 2023

Conversation

AlinsRan
Copy link
Contributor

@AlinsRan AlinsRan commented Nov 30, 2023

Description

Fixes #9618

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@AlinsRan AlinsRan closed this Nov 30, 2023
@AlinsRan AlinsRan reopened this Nov 30, 2023
@AlinsRan AlinsRan marked this pull request as ready for review November 30, 2023 08:04
Copy link
Contributor

@bzp2010 bzp2010 left a comment

Choose a reason for hiding this comment

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

This is only a temporary solution to get the API working again with a patch, but does not address its underlying issues.

We can fix the problem completely in a future PR.

nodes = setmetatable({}, array_mt)
}
local kvs = res.body.kvs
if #kvs >= 1 and not kvs[1].value then
Copy link
Contributor

Choose a reason for hiding this comment

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

not kvs[1].value

With all due respect, this is a completely false assumption. Why is the value null?

The lua-resty-etcd uses cjson.safe to parse JSON, so init_dir values that are not in JSON format will be parsed as nil.

Therefore, I think this is only a temporary solution that cannot solve the problem completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the previous logic, I did not modify it.
The previous non empty processing method was incorrect, I just improved it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, I think you are right. To improve this, I think we can add a judgment, such as or value="init_dir"

@bzp2010 bzp2010 merged commit 7836601 into apache:master Nov 30, 2023
50 of 65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: restart etcd admin api /apisix/admin/routes response is error
4 participants