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: fix fetch all service info from consul #8651

Merged
merged 15 commits into from
Feb 8, 2023

Conversation

Fabriceli
Copy link
Contributor

Description

Fixes # (issue)
As I mentioned in the email: https://lists.apache.org/thread/x0ksxx003tgqy4833vmsyq1mwwyc6wz7

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)

t/discovery/consul_dump.t Show resolved Hide resolved
t/discovery/consul_dump.t Outdated Show resolved Hide resolved
apisix/discovery/consul/init.lua Outdated Show resolved Hide resolved
apisix/discovery/consul/init.lua Outdated Show resolved Hide resolved
apisix/discovery/consul/init.lua Show resolved Hide resolved
@Fabriceli
Copy link
Contributor Author

@spacewander Please take a look at this PR, thanks

@spacewander
Copy link
Member

#8651 (comment)
It seems there are still indentation issues. Some places use 8-spaces indentation instead of 4.

@Fabriceli
Copy link
Contributor Author

#8651 (comment) It seems there are still indentation issues. Some places use 8-spaces indentation instead of 4.

I checked the code and make lint cant get WARN and ERROR, could you give some hint about this? I break a long code line to multiple lines, it will be made like follow by default:

log.error("connect consul: ", consul_server.consul_server_url,
        ", by service url: ", svc_url, ", with error: ", error_info)

@spacewander
Copy link
Member

#8651 (comment) It seems there are still indentation issues. Some places use 8-spaces indentation instead of 4.

I checked the code and make lint cant get WARN and ERROR, could you give some hint about this? I break a long code line to multiple lines, it will be made like follow by default:

log.error("connect consul: ", consul_server.consul_server_url,
        ", by service url: ", svc_url, ", with error: ", error_info)

The style guide tells us that we need to use 4-spaces indentation: https://github.com/apache/apisix/blob/master/CODE_STYLE.md#indentation

Some of the newly added code use 8-spaces indentation.

Another acceptable indentation is aligning the '(', like:

log.error("connect consul: ", consul_server.consul_server_url,
          ", by service url: ", svc_url, ", with error: ", error_info)

It is widely used in the APISIX source code.

I have tried several lints/fmtters. Unfortunately, they can't handle the second case. Therefore, so far we don't have an automatical mechanism to detect the indentation issue.

@Fabriceli
Copy link
Contributor Author

#8651 (comment) It seems there are still indentation issues. Some places use 8-spaces indentation instead of 4.

I checked the code and make lint cant get WARN and ERROR, could you give some hint about this? I break a long code line to multiple lines, it will be made like follow by default:

log.error("connect consul: ", consul_server.consul_server_url,
        ", by service url: ", svc_url, ", with error: ", error_info)

The style guide tells us that we need to use 4-spaces indentation: https://github.com/apache/apisix/blob/master/CODE_STYLE.md#indentation

Some of the newly added code use 8-spaces indentation.

Another acceptable indentation is aligning the '(', like:

log.error("connect consul: ", consul_server.consul_server_url,
          ", by service url: ", svc_url, ", with error: ", error_info)

It is widely used in the APISIX source code.

I have tried several lints/fmtters. Unfortunately, they can't handle the second case. Therefore, so far we don't have an automatical mechanism to detect the indentation issue.

Done, please re-check it, thanks

@Fabriceli
Copy link
Contributor Author

@tzssangglass could you have a look at this PR, thanks

@Fabriceli
Copy link
Contributor Author

@spacewander Could you request another committer to do the code review for this PR, thanks

@spacewander spacewander merged commit bbff579 into apache:master Feb 8, 2023
hongbinhsu added a commit to fitphp/apix that referenced this pull request Feb 13, 2023
* upstream/master:
  docs: change the file name to 'create-ssl.py'.If 'ssl.py' is used as … (apache#8623)
  feat: Body transformer plugin (apache#8766)
  fix: mocking plugin panic when response_example contain $ (apache#8810) (apache#8816)
  feat: file logger plugin support response body in variable (apache#8711)
  docs(getting-started): rewrite the install section (apache#8807)
  feat: allow each logger to define custom log format in its conf (apache#8806)
  fix(etcd): reloaded data may be in res.body.node (apache#8736)
  fix: fix fetch all service info from consul (apache#8651)
  docs: fix global-rule.md wrong curl  address (apache#8793)
  feat: stream subsystem support consul service discovery (apache#8696)
  chore(kafka-logger): support configuration `meta_refresh_interval` parameter (apache#8762)
  feat: ready to release 2.15.2 (apache#8783)
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.

3 participants