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

Support file-based properties #51

Merged
merged 4 commits into from
Sep 26, 2021
Merged

Support file-based properties #51

merged 4 commits into from
Sep 26, 2021

Conversation

supl
Copy link
Collaborator

@supl supl commented Sep 15, 2021

This PR makes ClientService support the following properties

  • scalar.dl.client.private_key_path
  • scalar.dl.client.cert_path
  • scalar.dl.client.tls.ca_root_cert_path

Developers can configure keys and certificates by giving related file paths.

@supl supl added enhancement New feature or request improvement labels Sep 15, 2021
@supl supl requested review from feeblefakie and jnmt September 15, 2021 07:47
@supl supl self-assigned this Sep 15, 2021
Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

Overall looking good. Thank you!
Left 1 comment. PTAL!

properties['scalar.dl.client.tls.ca_root_cert_pem'] =
properties['scalar.dl.client.tls.ca_root_cert_pem'] ||
fs.readFileSync(
properties['scalar.dl.client.tls.ca_root_cert_path'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need , at the end?
Also, should this be formatted?

Copy link
Collaborator Author

@supl supl Sep 26, 2021

Choose a reason for hiding this comment

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

Ah, that is because we use Google's ESLint config and it has this rule (comma-dangle)
https://github.com/google/eslint-config-google/blob/master/index.js#L195
It not only applies to the array and the object but also to the function arguments.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it is formatted properly, It's fine!

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

properties['scalar.dl.client.tls.ca_root_cert_pem'] =
properties['scalar.dl.client.tls.ca_root_cert_pem'] ||
fs.readFileSync(
properties['scalar.dl.client.tls.ca_root_cert_path'],
Copy link
Contributor

Choose a reason for hiding this comment

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

If it is formatted properly, It's fine!

@feeblefakie feeblefakie merged commit a72e4f2 into master Sep 26, 2021
@feeblefakie feeblefakie deleted the filebase-configuration branch September 26, 2021 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants