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 getPaths more robust #5113

Conversation

adamkpickering
Copy link
Member

Follow-up to #4160.

Signed-off-by: Adam Pickering <[email protected]>
@adamkpickering adamkpickering force-pushed the default-paths-on-rdctl-failure branch from a74a939 to 55e01fa Compare July 6, 2023 20:05
if (rdctlPath) {
const result = spawnSync(rdctlPath, ['paths'], { encoding: 'utf8' });
try {
const result = spawnSync(getRdctlPath(), ['paths'], { encoding: 'utf8' });

if (result.status !== 0) {
throw new Error(`rdctl paths failed: ${ JSON.stringify(result) }`);
Copy link
Contributor

Choose a reason for hiding this comment

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

This still doesn't make a lot of sense, because the error gets caught 3 lines later and the payload is dropped.

I think you can do what I suggested previously. Just delete the three lines

    if (result.status !== 0) {
      throw new Error(`rdctl paths failed: ${ JSON.stringify(result) }`);
    }

and set pathsData to that default for any failure.

Copy link
Member Author

@adamkpickering adamkpickering Jul 7, 2023

Choose a reason for hiding this comment

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

When spawnSync calls a command that returns a non-zero exit code, an error is not thrown. You have to check result.status (or maybe result.error?), so the if (result.status !== 0) {...} is needed. Same thing for a nonexistent command. See the following code for an example:

import child_process from 'child_process';

try {
  const result = child_process.spawnSync('cat', ['doesnotexist.txt'], {encoding: 'utf8'});
  console.log(JSON.stringify(result)); 
} catch (error) {
  console.log(`Got error ${error}`);
}

I've tried to figure out ways to simplify it with the needed if statement and a try-catch, but AFAICT this is as good as it gets. Let me know if you can see a better way 😄

Copy link
Contributor

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

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

Can be simplified, but it's on the right track.

@ericpromislow
Copy link
Contributor

We don't need this, going with #5127 instead

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.

2 participants