feat: sandbox deployment events #254

sdstolworthy · 2023-09-20T07:39:56Z

Issue #, if available:

Description of changes:
Refactors sandbox to extend EventEmitter so that sandbox can emit lifecycle events. In this PR, only one event has been implemented: successfulDeployment.

Clients of sandbox can now do:

const sandbox = new FileWatchingSandbox({});
sandbox.on('successfulDeployment', () => console.log('my event handler'))

This will allow for more flexible sandbox lifecycle side effects in the future.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

changeset-bot · 2023-09-20T07:39:59Z

🦋 Changeset detected

Latest commit: f0b70c3

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

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

packages/cli/src/commands/sandbox/sandbox_command.ts

…ered

packages/cli/src/commands/sandbox/sandbox-delete/sandbox_delete_command.test.ts

packages/cli/src/commands/sandbox/sandbox_command.ts

sobolk · 2023-09-22T18:06:11Z

packages/cli/src/commands/sandbox/sandbox_command.ts

+    private readonly createConfigGenerationCallback: (
+      appName?: string,
+      outDir?: string,
+      format?: ClientConfigFormat
+    ) => () => Promise<void>


Now I'm not sure if this is right direction, i.e. replacing types with "anonymous function".
The danger is, if everything accepts anonymous closures then it's hard to reason which puzzle is right for the other puzzle.
For simple cases closures are fine, here I think it's making it less obvious what's going on. The name of createConfigGenerationCallback saves the day a bit, but type would be better.

As for the unit testing part.
Why can't we just mock ClientConfigGeneratorAdapter in the test to provide different implementation and assert that callback is registered?
Mocking, aka altering types is fine in unit tests.

The problem is that the tests at that level are only testing the command interface level. So this refactor (or one of a couple very similar options) are the only way I'm able to assert that that specific method is being registered for that specific event. I agree that this current approach is not ideal.

Take a look here: https://github.com/sdstolworthy/samsara-cli/blob/sandbox-hooks/packages/cli/src/commands/sandbox/sandbox_command.test.ts#L25

This is why the test that is being described in the previous discussion is not possible at this level. This is not an integration test, it's a unit test. So, start has been overriden with a mock method.

Start is what is responsible for emitting the successfulDeployment event.

I think between the e2e test that already exists, the assertion that the on method is called, and the assertion test that start emits the successfulDeployment event provide sufficient coverage in this case.

I don't think the refactor here is a good direction. It allows us to make this very specific assertion, but at the cost of a poorly typed interface.

Ah ok. I understand now. This makes sense then.

But perhaps instead of anonymous createConfigGenerationCallback we need type SandboxEventListener ?

That makes sense. It could even be a creator function that returns

type SandboxEventHandlers = { successfulDeployment: VoidCallback[] }

And then we can register those events from the sandbox command.

yes please. This is a good opportunity to build better abstraction around it.

@sobolk done

packages/cli/src/commands/sandbox/sandbox_command_factory.ts

packages/cli/src/commands/sandbox/sandbox_command.ts

edwardfoyle · 2023-09-22T19:56:01Z

packages/cli/src/commands/sandbox/sandbox_command.ts

@@ -14,6 +17,22 @@ export type SandboxCommandOptions = {
  profile: string | undefined;
 };

+export type EventHandler = () => void;


Based on this line it looks like the return type here should be Promise<void>?

node:EventEmitter does not accept Promise<void>. It is fire and forget. See here: #254 (comment)

edwardfoyle · 2023-09-22T19:59:10Z

packages/sandbox/src/file_watching_sandbox.ts

+   * @inheritdoc
+   */
+  override emit(eventName: SandboxEvents, ...args: unknown[]): boolean {
+    return super.emit(eventName, args);


Where are the async handlers actually awaited?

That's the cool thing, they aren't :).
See comment above. EventEmitter is fire and forget only. It does not await promises. This makes sense, its whole job is simply to call the methods. Handling errors or async logic is the responsibility of the registered handler.

What if the process exits before the promise finishes? This could actually be a real problem for our e2e tests that sigints the process the moment the deployment is complete

Node drains event loop, unless there's abnormal termination.

If we want to be sure this won't cause e2e failures, you'll need to copy these changes to a branch in the repo and run e2e on that (because AWS creds aren't shared with forks)

Well the windows e2e tests failed with the exact error I would expect if it failed to write the client config file...
https://github.com/aws-amplify/samsara-cli/actions/runs/6278705272/job/17053506116?pr=275#step:7:319

Mac and Linux e2e passed but perhaps because the promise finished in time on those machines

Node drains event loop, unless there's abnormal termination.

Actually I bet this is it. Windows doesn't have SIGINT, instead we have to hard kill the process. See https://github.com/aws-amplify/samsara-cli/blob/main/packages/integration-tests/src/process-controller/process_controller.ts#L74-L82

If I'm reading that code right, this is failing because of basically modal behavior of our integration suite with Windows, and the failure does not necessarily indicate that this would fail under normal circumstances?

perhaps Windows script can take a nap before the kill.

Yeah that's the current thought for the workaround. Not ideal, but serviceable.

* feat: sandbox options add format * test: add format to available options * fix: sandbox command option format * add changeset * chore: update API.md * refactor: move get_config_path to client-config * chore: type and lint * chore: update API.md * chore: import formatChoices to sandbox * chore: change template literal * chore: update types, use Enum * chore: update types for FormatOption * chore: fix type * chore: move getConfigPath to internal api * chore: rename getClientConfigPath * chore: rename FormatChoice * chore: update API.md * chore: add index.js to internal.ts * chore: remove configFileName and formatChoices from API * fix: test * sandbox: initial implementation of deployment hooks * sandbox: simplify hook registration example * lint: fix lints * refactor: use events instead of hooks * api: update api description files * logging: added debug logging * tests: fix broken test * sandbox: base event handler on node:events EventEmitter * tests: fix broken test * chore: move getConfigPath from GenerateConfigCommand to generateClientConfigToFile * Update packages/client-config/src/paths/get_client_config_path.ts Co-authored-by: Kamil Sobol <[email protected]> * chore: rename FormatChoice to ClientConfigFormat * refactor: generateClientConfigToFile to accept out and format * test: update tests for generate config and sandbox * Update packages/cli/src/commands/sandbox/sandbox_command.ts Co-authored-by: Edward Foyle <[email protected]> * test: misc var change * chore: move ClientConfigFormat to public API * chore: update API.md * test: add test for getClientConfigPath * test: use path.join to resolve windows slash * test: use path.join to resolve windows slash * chore: fix linting error * chore: fix linting error * chore: remove defaultOptions * test: add asserts for both out and format * test: add absolut path test * fix: throw error if provided file path * fix: broken test * fix: integrate format into event callback * api: update api description files * fix: address pr comments * chore: rename --out to --outDir * fix: use lstatSync to detect file path * chore: update API.md * chore: changeset * chore: remove un-used var * fix: package-lock resolve registry * chore: ignore spell of lstat * chore: refactor mock-fs with node:mock * chore: remove un-used var * chore: make getClientConfigPath async * fix: path for windows * chore: remove auto-generated changelogs * fix: remove references to deleted events * api: update api description files * refactor: rename event, move generate-config-adapter * refactor: remove sandbox config adapter * refactor: use args.out directly for config write path * refactor: use exported getClientConfigPath from client-config package * refactor: use mocks for test * fix: explicitly export getClientConfigPath * Update packages/sandbox/src/file_watching_sandbox.ts Co-authored-by: Kamil Sobol <[email protected]> * fix: remove unused import * Update packages/cli/src/commands/sandbox/sandbox_command.ts Co-authored-by: Amplifiyer <[email protected]> * refactor: extract getBackendIdentifier * test: add test to validate that successfulDeployment event is emitted * test: refactor to allow asserting that a specific callback was registered * fix: remove unused import * refactor: creator method for registering sandbox events * fix: remove unused import * fix: fix naming, refactor for readability --------- Co-authored-by: MJ☔ <[email protected]> Co-authored-by: MJ Zhang <[email protected]> Co-authored-by: Kamil Sobol <[email protected]> Co-authored-by: Edward Foyle <[email protected]> Co-authored-by: Amplifiyer <[email protected]>

…x-hooks

…li into sandbox-hooks

edwardfoyle · 2023-09-27T17:09:11Z

packages/integration-tests/src/process-controller/process_controller.ts

@@ -72,6 +72,8 @@ export class ProcessController {
      if (typeof currentInteraction.payload === 'string') {
        if (currentInteraction.payload === CONTROL_C) {
          if (process.platform.startsWith('win')) {
+            // Wait X milliseconds before sending kill in hopes of draining the node event queue
+            await new Promise((resolve) => setTimeout(resolve, 5000));


approved as long as we add a GH issue to the backlog to track this. Bonus points for linking to that issue in the comment here

feat: add deployment events to sandbox

zhamujun added 24 commits September 15, 2023 15:11

feat: sandbox options add format

8bbde6d

test: add format to available options

7ad12f1

fix: sandbox command option format

0ef8755

Merge branch 'main' into sandbox-option-format

1d7c2f9

add changeset

92a49e7

chore: update API.md

0063a4d

refactor: move get_config_path to client-config

7a77b27

chore: type and lint

a45c889

chore: update API.md

5c58fe4

Merge branch 'main' into sandbox-option-format

eefeaf0

chore: import formatChoices to sandbox

bd0f86e

Merge branch 'main' into sandbox-option-format

c861be3

chore: change template literal

6309b05

chore: update types, use Enum

87530a6

chore: update types for FormatOption

f176750

chore: fix type

1986cd0

chore: move getConfigPath to internal api

ea110b3

chore: rename getClientConfigPath

516bb8a

chore: rename FormatChoice

c1f1c0f

chore: update API.md

40d20fb

chore: add index.js to internal.ts

cf25eac

chore: remove configFileName and formatChoices from API

75f6be7

fix: test

cddb7c2

Merge branch 'main' into sandbox-option-format

94625e7

sandbox: initial implementation of deployment hooks

0feeafe

sdstolworthy force-pushed the sandbox-hooks branch from 2c9cd5a to 0feeafe Compare September 20, 2023 07:41

github-advanced-security bot found potential problems Sep 20, 2023

View reviewed changes

packages/cli/src/commands/sandbox/sandbox_command.ts Fixed Show fixed Hide fixed

sdstolworthy added 2 commits September 20, 2023 01:46

sandbox: simplify hook registration example

05075f9

lint: fix lints

bdb2f65

sdstolworthy dismissed sobolk’s stale review via 30f5572 September 22, 2023 16:06

sdstolworthy added 2 commits September 22, 2023 10:15

refactor: extract getBackendIdentifier

a816b14

test: add test to validate that successfulDeployment event is emitted

6c201eb

sobolk mentioned this pull request Sep 22, 2023

generate client code command #265

Merged

test: refactor to allow asserting that a specific callback was regist…

bd5d258

…ered

github-advanced-security bot found potential problems Sep 22, 2023

View reviewed changes

sobolk reviewed Sep 22, 2023

View reviewed changes

sdstolworthy added 4 commits September 22, 2023 12:47

fix: remove unused import

dcbcbc7

Merge branch 'main' into sandbox-hooks

008406e

refactor: creator method for registering sandbox events

67f118a

fix: remove unused import

afd1012

sobolk reviewed Sep 22, 2023

View reviewed changes

packages/cli/src/commands/sandbox/sandbox_command_factory.ts Outdated Show resolved Hide resolved

packages/cli/src/commands/sandbox/sandbox_command.ts Outdated Show resolved Hide resolved

fix: fix naming, refactor for readability

6607c76

sobolk previously approved these changes Sep 22, 2023

View reviewed changes

Amplifiyer previously approved these changes Sep 22, 2023

View reviewed changes

edwardfoyle reviewed Sep 22, 2023

View reviewed changes

edwardfoyle added the run-e2e Label that will include e2e tests in PR checks workflow label Sep 22, 2023

edwardfoyle removed the run-e2e Label that will include e2e tests in PR checks workflow label Sep 22, 2023

Merge branch 'main' of github.com:aws-amplify/samsara-cli into sandbo…

7885a8c

…x-hooks

sdstolworthy dismissed stale reviews from Amplifiyer and sobolk via 7885a8c September 27, 2023 14:51

sdstolworthy added 2 commits September 27, 2023 08:36

fix: add delay

2f700db

Merge branch 'e2e-sandbox-events' of github.com:aws-amplify/samsara-c…

f0b70c3

…li into sandbox-hooks

sobolk approved these changes Sep 27, 2023

View reviewed changes

edwardfoyle approved these changes Sep 27, 2023

View reviewed changes

sdstolworthy mentioned this pull request Sep 27, 2023

Handle SIGINT on windows #300

Open

sdstolworthy merged commit ee3d55f into aws-amplify:main Sep 27, 2023
37 checks passed

sdstolworthy deleted the sandbox-hooks branch September 27, 2023 17:28

abhi7cr pushed a commit that referenced this pull request Sep 27, 2023

feat: sandbox deployment events (#254)

573deaf

feat: add deployment events to sandbox

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: sandbox deployment events #254

feat: sandbox deployment events #254

sdstolworthy commented Sep 20, 2023 •

edited

Loading

changeset-bot bot commented Sep 20, 2023 •

edited

Loading

sobolk Sep 22, 2023

sdstolworthy Sep 22, 2023 •

edited

Loading

sobolk Sep 22, 2023

sdstolworthy Sep 22, 2023

sobolk Sep 22, 2023

sdstolworthy Sep 22, 2023

edwardfoyle Sep 22, 2023

sdstolworthy Sep 22, 2023

edwardfoyle Sep 22, 2023

sdstolworthy Sep 22, 2023

edwardfoyle Sep 22, 2023 •

edited

Loading

sobolk Sep 22, 2023

edwardfoyle Sep 22, 2023

edwardfoyle Sep 22, 2023 •

edited

Loading

edwardfoyle Sep 22, 2023 •

edited

Loading

sdstolworthy Sep 22, 2023

sobolk Sep 22, 2023

sdstolworthy Sep 22, 2023

edwardfoyle Sep 27, 2023

sdstolworthy Sep 27, 2023

feat: sandbox deployment events #254

feat: sandbox deployment events #254

Conversation

sdstolworthy commented Sep 20, 2023 • edited Loading

changeset-bot bot commented Sep 20, 2023 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

sdstolworthy Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardfoyle Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardfoyle Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

edwardfoyle Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdstolworthy commented Sep 20, 2023 •

edited

Loading

changeset-bot bot commented Sep 20, 2023 •

edited

Loading

sdstolworthy Sep 22, 2023 •

edited

Loading

edwardfoyle Sep 22, 2023 •

edited

Loading

edwardfoyle Sep 22, 2023 •

edited

Loading

edwardfoyle Sep 22, 2023 •

edited

Loading