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

Make header methods and config public #191

Closed
wants to merge 2 commits into from
Closed

Make header methods and config public #191

wants to merge 2 commits into from

Conversation

quangIO
Copy link
Contributor

@quangIO quangIO commented Mar 30, 2022

The PR proposes to make the config and helper methods inside Headers public. The current SxgWorker requires user to gather all certificates before hand. However, it may not be the case when a system stores certificates elsewhere or want to construct SxgWorker dynamically.

@google-cla
Copy link

google-cla bot commented Mar 30, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@antiphoton
Copy link
Collaborator

Thanks for you contribution.

  1. Making Headers public LGTM, but please split the headers change and worker config change into two PRs.
  2. To construct SxgWorker dynamically, please use SxgWorker::new(config, cert, issuer), because this constructor also internally does some parsing work. If you need to construct a Config with certificates in formats other than PEM, you are welcome to set the PR to add more constructors to Config.

Copy link
Collaborator

@twifkak twifkak left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I'll let @antiphoton review in full. I just wanted to leave one small (optional) suggestion.

Comment on lines +4 to +7
.log
.vscode
.idea
.DS_store
Copy link
Collaborator

Choose a reason for hiding this comment

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

I recommend limiting .gitignore to project-specific artifacts, and using ~/.config/git/ignore for tool-related artifacts across projects.

@quangIO
Copy link
Contributor Author

quangIO commented Mar 31, 2022

Thanks for the code review. I will close the PR in favour of #198 and #199.

@quangIO quangIO closed this Mar 31, 2022
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