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

fix when you create-amplify absolute path, raised error #1071

Merged

Conversation

magisystem0408
Copy link
Contributor

@magisystem0408 magisystem0408 commented Feb 25, 2024

Problem

**Issue number, if available: **
fixes: #827

Related pull requests: #857

Changes

Corresponding docs PR, if applicable:

when you do create-amplify absolute path (ex. '/user/test'), raised error and log.

Validation

Checklist

  • If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.
  • If this PR requires a change to the Project Architecture README, I have included that update in this PR.
  • If this PR requires a docs update, I have linked to that docs PR above.
  • If this PR modifies E2E tests, makes changes to resource provisioning, or makes SDK calls, I have run the PR checks with the run-e2e label set.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

changeset-bot bot commented Feb 25, 2024

🦋 Changeset detected

Latest commit: 7cc3407

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
create-amplify Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@magisystem0408 magisystem0408 changed the title fix create-amplify with absolute path fix when you create-amplify absolute path, raised error Feb 25, 2024
sobolk
sobolk previously approved these changes Feb 26, 2024
@sobolk
Copy link
Member

sobolk commented Feb 26, 2024

Note to other reviewers: this change was discussed in #857 and got moved to new PR.

try {
await fsp.mkdir(projectRoot, { recursive: true });
} catch (err) {
if (path.isAbsolute(projectRoot)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Amplifiyer
I can't think of a case at the moment where projectRoot not be an absolute path, but I put it in explicitly.

Is it necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

if there is no situation where projectRoot wouldn't be an absolute path than this if statement is irrelevant and we should throw an AmplifyError with the right messaging here.

cc: @edwardfoyle

Copy link
Member

Choose a reason for hiding this comment

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

Based on the upstream validation, I would say we can remove this conditional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you feedback !!! @edwardfoyle @Amplifiyer

I try to do this task!

  • remove condition
  • add AmplifyError (change test case)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Amplifiyer @edwardfoyle

I came up with three error pattern.
should select plan3 right?

// plan1: use printer
try {
    await fsp.mkdir(projectRoot, {recursive: true});
} catch (err) {
    printer.log(
        `Failed to create directory at ${projectRoot}. Ensure this is the correct path and you have write permissions to this location.`,
        LogLevel.ERROR
    );
    throw err;
}

// plan2: direct throw new Error
try {
    await fsp.mkdir(projectRoot, {recursive: true});
} catch (err) {
    throw new Error(`Failed to create directory at ${projectRoot}. Ensure this is the correct path and you have write permissions to this location.`);
}

// plan3: use AmplifyUserError
import {AmplifyUserError} from '@aws-amplify/platform-core';

try {
    await fsp.mkdir(projectRoot, {recursive: true});
} catch (err) {
    throw new AmplifyUserError('MultipleSingletonResourcesError', {
        message:
            `Failed to create directory at ${projectRoot}. Ensure this is the correct path and you have write permissions to this location.`,
        resolution: 'Change relative path or Absolute path under root',
    });
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes plan3 is correct. Make the error name something like ProjectDirectoryCreateError and change the message and resolution to something like

try {
    await fsp.mkdir(projectRoot, {recursive: true});
} catch (err) {
    throw new AmplifyUserError('MultipleSingletonResourcesError', {
        message:
            `Failed to create project directory`,
        resolution: 'Ensure that ${projectRoot} is the correct path and you have write permissions to this location.',
    });
}

edwardfoyle
edwardfoyle previously approved these changes Feb 28, 2024
@magisystem0408
Copy link
Contributor Author

magisystem0408 commented Feb 28, 2024

@Amplifiyer

I modified the test code and AmplifyError with reference to the following.
please check🙇‍♀️🙇‍♀️🙇‍♀️🙇‍♀️🙇‍♀️

const actual = AmplifyError.fromStderr(sampleStderr);
assert.deepStrictEqual(actual?.name, testError.name);
assert.deepStrictEqual(actual?.classification, testError.classification);
assert.deepStrictEqual(actual?.message, testError.message);
assert.deepStrictEqual(actual?.details, testError.details);
assert.deepStrictEqual(actual?.cause?.name, testError.cause?.name);
assert.deepStrictEqual(actual?.cause?.message, testError.cause?.message);
});

Comment on lines 90 to 109
(err: Error) => {
const sampleStderr = `some random stderr
before the actual error message
${util.inspect(err, { depth: null })}
and some after the error message`;

const actual = AmplifyError.fromStderr(sampleStderr);
assert.deepStrictEqual(actual?.name, expectedError.name);
assert.deepStrictEqual(
actual?.classification,
expectedError.classification
);
assert.deepStrictEqual(actual?.message, expectedError.message);
assert.deepStrictEqual(actual?.details, expectedError.details);
assert.deepStrictEqual(actual?.cause?.name, expectedError.cause?.name);
assert.deepStrictEqual(
actual?.cause?.message,
expectedError.cause?.message
);
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need all this. Just assert on the err that the name, message and resolution is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Amplifiyer
thank you feedback!!!!!
I try change test case

Comment on lines 88 to 107
await assert.rejects(
() => getProjectRoot(),
(err: Error) => {
const sampleStderr = `some random stderr
before the actual error message
${util.inspect(err, { depth: null })}
and some after the error message`;

const actual = AmplifyError.fromStderr(sampleStderr);
assert.deepStrictEqual(actual?.name, expectedError.name);
assert.deepStrictEqual(actual?.message, expectedError.message);
assert.deepStrictEqual(actual?.resolution, expectedError.resolution);
return true;
}
);
assert.equal(fsMkDirSyncMock.mock.callCount(), 1);
assert.equal(
fsMkDirSyncMock.mock.calls[0].arguments[0],
path.resolve(userInput)
);
Copy link
Contributor

@Amplifiyer Amplifiyer Mar 1, 2024

Choose a reason for hiding this comment

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

something like below

Suggested change
await assert.rejects(
() => getProjectRoot(),
(err: Error) => {
const sampleStderr = `some random stderr
before the actual error message
${util.inspect(err, { depth: null })}
and some after the error message`;
const actual = AmplifyError.fromStderr(sampleStderr);
assert.deepStrictEqual(actual?.name, expectedError.name);
assert.deepStrictEqual(actual?.message, expectedError.message);
assert.deepStrictEqual(actual?.resolution, expectedError.resolution);
return true;
}
);
assert.equal(fsMkDirSyncMock.mock.callCount(), 1);
assert.equal(
fsMkDirSyncMock.mock.calls[0].arguments[0],
path.resolve(userInput)
);
await assert.throws(
() => getProjectRoot(),
expectedError
);
assert.equal(fsMkDirSyncMock.mock.callCount(), 1);
assert.equal(
fsMkDirSyncMock.mock.calls[0].arguments[0],
path.resolve(userInput)
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Amplifiyer

I've made a few modifications.
at d7226b6

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 came up with idea.

const expectedError = new AmplifyUserError('ProjectDirectoryCreateError', {
      message: `Failed to create project directory`,
      resolution: `Ensure that ${
        userInput
      } is the correct path and you have write permissions to this location.`,
    });
    fsMkDirSyncMock.mock.mockImplementationOnce(() => {
      throw expectedError;
    });

    await assert.rejects(() => getProjectRoot(), {
      name: expectedError.name,
      message: expectedError.message,
    });

@Amplifiyer Amplifiyer merged commit c6a65d5 into aws-amplify:main Mar 18, 2024
23 of 24 checks passed
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.

providing path with/ at the start caused npm create amplify@latest to crash with stack trace
4 participants