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
some optimizations for cert.go #9404
Conversation
Signed-off-by: ls0f <lovedboy.tk@qq.com>
Codecov Report
@@ Coverage Diff @@
## master #9404 +/- ##
==========================================
- Coverage 46.19% 45.76% -0.43%
==========================================
Files 218 220 +2
Lines 25917 26164 +247
==========================================
+ Hits 11972 11975 +3
- Misses 12290 12531 +241
- Partials 1655 1658 +3
Continue to review full report at Codecov.
|
@ls0f Can we add some description to the PR for the need of the change? |
@@ -100,11 +99,10 @@ func GetTLSCertificateDataPath() string { | |||
// filesystem. If ARGOCD_SSH_DATA_PATH environment is set, path is taken from | |||
// there, otherwise the default will be returned. | |||
func GetSSHKnownHostsDataPath() string { | |||
envPath := os.Getenv(common.EnvVarSSHDataPath) | |||
if envPath != "" { | |||
return envPath + "/" + common.DefaultSSHKnownHostsName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use filepath.Join to get the final dst path
@@ -332,7 +327,7 @@ func GetCertificateForConnect(serverName string) ([]string, error) { | |||
// mount. This function makes sure that the path returned actually contain | |||
// at least one valid certificate, and no invalid data. | |||
func GetCertBundlePathForRepository(serverName string) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use filepath.Join to get the final dst path too.
@@ -302,9 +300,6 @@ func ServerNameWithoutPort(serverName string) string { | |||
// consider it an error and just return empty data. | |||
func GetCertificateForConnect(serverName string) ([]string, error) { | |||
dataPath := GetTLSCertificateDataPath() | |||
if !strings.HasSuffix(dataPath, "/") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since use file path to combine dir, it's unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
i will update |
@@ -88,8 +88,7 @@ func IsValidHostname(hostname string, fqdn bool) bool { | |||
// filesystem. If ARGOCD_TLS_DATA_PATH environment is set, path is taken from | |||
// there, otherwise the default will be returned. | |||
func GetTLSCertificateDataPath() string { | |||
envPath := os.Getenv(common.EnvVarTLSDataPath) | |||
if envPath != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more clean code i think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
No description provided.