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

feat: expose TLSConfig for config api #2245

Merged
merged 3 commits into from
Mar 15, 2023
Merged

feat: expose TLSConfig for config api #2245

merged 3 commits into from
Mar 15, 2023

Conversation

sekfung
Copy link
Contributor

@sekfung sekfung commented Mar 10, 2023

What this PR does:
expose TLSConfig for config api

Which issue(s) this PR fixes:
Fixes #

You should pay attention to items below to ensure your pr passes our ci test
We do not merge pr with ci tests failed

  • All ut passed (run 'go test ./...' in project root)
  • After go-fmt ed , run 'go fmt project' using goland.
  • Golangci-lint passed, run 'sudo golangci-lint run' in project root.
  • Your new-created file needs to have apache license at the top, like other existed file does.
  • All integration test passed. You can run integration test locally (with docker env). Clone our dubbo-go-samples project and replace the go.mod to your dubbo-go, and run 'sudo sh start_integration_test.sh' at root of samples project root. (M1 Slice is not Support)

Signed-off-by: sekfung <sekfung.lau@gmail.com>
@justxuewei
Copy link
Member

It seems that a TLS test fails. Please check and fix it.

@sekfung
Copy link
Contributor Author

sekfung commented Mar 10, 2023

OK, I will fix it

@sekfung
Copy link
Contributor Author

sekfung commented Mar 13, 2023

@justxuewei hi, plz review the changes when you are free, thx

@justxuewei
Copy link
Member

justxuewei commented Mar 13, 2023

It looks like your commits modify tons of comments, why? What formatting tools are you using? Plus, please add an Apache header for config/tls_config_test.go.

@sekfung
Copy link
Contributor Author

sekfung commented Mar 13, 2023

  1. go fmt ./... and import-formatter. That is incredible, the prev commit has passed the format check

  2. I will add the license header

@justxuewei
Copy link
Member

The comments have been heavily modified, which is unrelated to this pull request's topic, so we need to revert them back. I'll run formatting tools on my local environment to investigate what's causing the issue. Is your go version 1.20?

@sekfung
Copy link
Contributor Author

sekfung commented Mar 13, 2023

yes, go version 1.20

Signed-off-by: sekfung <sekfung.lau@gmail.com>
Signed-off-by: sekfung <sekfung.lau@gmail.com>
@sonarcloud
Copy link

sonarcloud bot commented Mar 13, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sekfung
Copy link
Contributor Author

sekfung commented Mar 13, 2023

Unable to load X509 Key Pair open /home/runner/work/dubbo-go-samples/dubbo-go-samples/tls/x509/client1_cert.pem: no such file or directory

It seems that the cert file path was not correct

Does the CI workflow work correctly? I had not changed the logic which read cert. Just expose tls config for config api

Other PR has the same error Unable to load X509 Key Pair

@justxuewei
Copy link
Member

Unable to load X509 Key Pair open /home/runner/work/dubbo-go-samples/dubbo-go-samples/tls/x509/client1_cert.pem: no such file or directory

It seems that the cert file path was not correct

Does the CI workflow work correctly? I had not changed the logic which read cert. Just expose tls config for config api

Other PR has the same error Unable to load X509 Key Pair

The directory for certificates is hard-coded. It works for the integrated tests of dubbo-go-sample, but it does not work for dubbo-go. I'll let you know if it is fixed.

@sekfung
Copy link
Contributor Author

sekfung commented Mar 13, 2023

Unable to load X509 Key Pair open /home/runner/work/dubbo-go-samples/dubbo-go-samples/tls/x509/client1_cert.pem: no such file or directory

It seems that the cert file path was not correct
Does the CI workflow work correctly? I had not changed the logic which read cert. Just expose tls config for config api
Other PR has the same error Unable to load X509 Key Pair

The directory for certificates is hard-coded. It works for the integrated tests of dubbo-go-sample, but it does not work for dubbo-go. I'll let you know if it is fixed.

ok, thx for your kindness

Copy link
Member

@justxuewei justxuewei left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks @sekfung!

@justxuewei
Copy link
Member

@AlexStocks @zhaoyunxing92 PTAL

@justxuewei justxuewei merged commit b4ad6a3 into apache:3.0 Mar 15, 2023
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.

None yet

3 participants