Skip to content

Conversation

@mingqishao
Copy link
Contributor

Description of your changes

This is following up some comments on another PR: #364 after PR merged.

I have:

  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer

caBundle, ok := os.LookupEnv("CA_BUNDLE")
if ok && caBundle == "" {
err := errors.New("environment variable CA_BUNDLE should not be empty")
klog.ErrorS(err, "failed to validate system variables")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not return the err here? I am not sure why we just log this as user can't see the error log. I don't think an empty env is valid. I assume that not setting it means using os default but setting it as "" ambiguous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that is a copy/paste bug. I fixed that and added unit test in this commit.

return nil, err
}

hubConfig.TLSClientConfig.CAFile = caBundle
Copy link
Contributor

Choose a reason for hiding this comment

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

we should only set caBundle or caData

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in this commit

@ryanzhang-oss ryanzhang-oss merged commit 6107234 into main May 30, 2023
@zhiying-lin zhiying-lin deleted the minsha/cadata-optional-followup branch August 16, 2023 08:42
weng271190436 pushed a commit to weng271190436/fleet that referenced this pull request Dec 8, 2025
Signed-off-by: Zhiying Lin <zhiyingl456@gmail.com>
Co-authored-by: Ryan Zhang <yangzhangrice@hotmail.com>
@weng271190436 weng271190436 mentioned this pull request Dec 8, 2025
1 task
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