Skip to content

fix: fix fetch all service info from consul#8651

Merged
spacewander merged 15 commits into
apache:masterfrom
Fabriceli:feat/fix_consul_service
Feb 8, 2023
Merged

fix: fix fetch all service info from consul#8651
spacewander merged 15 commits into
apache:masterfrom
Fabriceli:feat/fix_consul_service

Conversation

@Fabriceli

@Fabriceli Fabriceli commented Jan 10, 2023

Copy link
Copy Markdown
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)

spacewander
spacewander reviewed Jan 12, 2023
View reviewed changes
Comment thread t/discovery/consul_dump.t
Comment thread t/discovery/consul_dump.t Outdated
Comment thread apisix/discovery/consul/init.lua Outdated
Comment thread apisix/discovery/consul/init.lua Outdated
Comment thread apisix/discovery/consul/init.lua
spacewander
spacewander reviewed Jan 18, 2023
View reviewed changes
Comment thread apisix/discovery/consul/init.lua Outdated
@Fabriceli

Fabriceli commented Jan 20, 2023

Copy link
Copy Markdown
Contributor Author

@spacewander Please take a look at this PR, thanks

@spacewander

spacewander commented Jan 28, 2023

Copy link
Copy Markdown
Member

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

@Fabriceli

Fabriceli commented Jan 30, 2023

Copy link
Copy Markdown
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

spacewander commented Jan 31, 2023

Copy link
Copy Markdown
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

Fabriceli commented Jan 31, 2023

Copy link
Copy Markdown
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

Fabriceli commented Feb 1, 2023

Copy link
Copy Markdown
Contributor Author

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

@Fabriceli

Fabriceli commented Feb 8, 2023

Copy link
Copy Markdown
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
@hongbinhsu
Merge branch 'upstream/master' into github/master
a7e20e7 
* 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)
@Fabriceli Fabriceli mentioned this pull request Feb 13, 2023
Closed
@dyrnq dyrnq mentioned this pull request Feb 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spacewander spacewander spacewander approved these changes
@soulbird soulbird soulbird approved these changes

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

@Fabriceli @spacewander @soulbird