Skip to content

feat: configure LDP server with certificate + some cleanup #72

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

Merged
merged 10 commits into from
Jun 28, 2024

Conversation

maliroteh-sf
Copy link
Contributor

What does this PR do?

  • Add the ability to configure lwc-dev-server server with certificate
  • Some refactoring and code cleanup

What issues does this PR fix or reference?

@W-15906501@

@maliroteh-sf maliroteh-sf requested a review from a team as a code owner June 25, 2024 21:46
Copy link
Collaborator

@khawkins khawkins left a comment

Choose a reason for hiding this comment

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

I think, other than the outstanding certificate discussions, this looks good to me.

Copy link
Collaborator

@khawkins khawkins left a comment

Choose a reason for hiding this comment

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

The changes look good to me overall. Just the question of binary cert data (which you have in progress) and the configuration of the port.

// If we have not previously generated the cert files then go ahead and do so
if (!fs.existsSync(targetFile)) {
if (platform === Platform.ios) {
fs.writeFileSync(targetFile, data.derCertificate, { encoding: 'binary' });
Copy link
Collaborator

Choose a reason for hiding this comment

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

So here I'd drop the options specifying encoding, because that's an inherently textual configuration, and doesn't apply to a raw Buffer.

Copy link
Collaborator

@khawkins khawkins left a comment

Choose a reason for hiding this comment

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

Just the minor comment around saving the DER file, but otherwise this looks good to me!

@maliroteh-sf maliroteh-sf merged commit fa4c3e8 into salesforcecli:main Jun 28, 2024
1 check passed
@maliroteh-sf maliroteh-sf deleted the cert branch June 28, 2024 21:59
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