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

Automation API nodejs #5347

Merged
merged 35 commits into from Oct 8, 2020
Merged

Automation API nodejs #5347

merged 35 commits into from Oct 8, 2020

Conversation

EvanBoyle
Copy link
Contributor

@EvanBoyle EvanBoyle commented Sep 14, 2020

This is the nodejs implementation of the Automation API. This isn't at complete parity with the Go implementation, but is enough of an initial impl to get the work landed. This includes:

  • Workspace interface and LocalWorkspace impl
  • Stack
  • Opaque error design (allows users to detect cases like conflicts without having users take a dependency on today's CLI output)
  • First pass at typedoc comments (more to come in the future as we invest in a pulumi learn page on pulumi.com).
  • Support for local and inline programs (adding host mode for nodejs via server.ts).

Contributes to #3901

@EvanBoyle
Copy link
Contributor Author

@pgavlin @justinvp I could use an initial set of eyes on this for an API review. I'd like feedback on the surface area, and any suggestions to make it more idiomatic. Could you take a look at and drop some notes on the following:

  1. Workspace interface and LocalWorkspace surface area.
  2. Stack
  3. Example usage in the tests.
  4. Any comments, ideas, or reference libraries you might point to for the structured error design.

Copy link
Member

@justinvp justinvp left a comment

Choose a reason for hiding this comment

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

Nice to see this coming together! Here's some initial feedback.

I am still mulling on the overall design. I'm going to spend a little more time noodling and thinking about it and will let you know if I have any other concrete suggestions. Will try to get that to you ASAP.

sdk/nodejs/Makefile Outdated Show resolved Hide resolved
sdk/nodejs/x/automation/cmd.ts Show resolved Hide resolved
sdk/nodejs/x/automation/config.ts Outdated Show resolved Hide resolved
sdk/nodejs/x/automation/localWorkspace.ts Outdated Show resolved Hide resolved
sdk/nodejs/x/automation/workspace.ts Show resolved Hide resolved
sdk/nodejs/x/automation/stackSettings.ts Outdated Show resolved Hide resolved
sdk/nodejs/x/automation/localWorkspace.ts Outdated Show resolved Hide resolved
sdk/nodejs/x/automation/localWorkspace.ts Outdated Show resolved Hide resolved
sdk/nodejs/x/automation/stack.ts Outdated Show resolved Hide resolved
sdk/nodejs/x/automation/cmd.ts Outdated Show resolved Hide resolved
@EvanBoyle EvanBoyle force-pushed the evan/autoNode branch 4 times, most recently from 27efe13 to 643d271 Compare October 7, 2020 02:28
@EvanBoyle EvanBoyle requested review from justinvp and removed request for pgavlin October 7, 2020 07:08
@EvanBoyle EvanBoyle marked this pull request as ready for review October 7, 2020 07:15
@EvanBoyle
Copy link
Contributor Author

@justinvp please take another look. I believe things are in a shippable state at this point. I will definitely be continuing to iterate and land new features over the next week or two, but I believe this hits all of the broad strokes.

Copy link
Member

@justinvp justinvp left a comment

Choose a reason for hiding this comment

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

I'm still looking this over (will continue looking later this evening), in the meantime, here are some initial comments.

sdk/nodejs/x/automation/errors.ts Outdated Show resolved Hide resolved
sdk/nodejs/x/automation/errors.ts Outdated Show resolved Hide resolved
sdk/nodejs/tests/testmode.spec.ts Outdated Show resolved Hide resolved
sdk/nodejs/x/automation/localWorkspace.ts Outdated Show resolved Hide resolved
sdk/nodejs/x/automation/localWorkspace.ts Outdated Show resolved Hide resolved
sdk/nodejs/x/automation/stack.ts Outdated Show resolved Hide resolved
sdk/nodejs/x/automation/stack.ts Outdated Show resolved Hide resolved
sdk/nodejs/x/automation/stack.ts Outdated Show resolved Hide resolved
sdk/nodejs/x/automation/stack.ts Outdated Show resolved Hide resolved
sdk/nodejs/x/automation/workspace.ts Outdated Show resolved Hide resolved
@EvanBoyle
Copy link
Contributor Author

@justinvp thanks for taking a look! I've updated everything accordingly. Let me know if you think we need any additional changes to get this landed!

Copy link
Member

@justinvp justinvp left a comment

Choose a reason for hiding this comment

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

Some more comments, but otherwise LGTM for getting this initial version landed.

sdk/nodejs/x/automation/workspace.ts Show resolved Hide resolved
sdk/nodejs/x/automation/localWorkspace.ts Outdated Show resolved Hide resolved
sdk/nodejs/x/automation/localWorkspace.ts Outdated Show resolved Hide resolved
*
* @param opts Options used to configure the Workspace
*/
public static async create(opts: LocalWorkspaceOptions): Promise<LocalWorkspace> {
Copy link
Member

Choose a reason for hiding this comment

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

The one thing I keep wondering about is whether we can make it more intuitive/obvious which methods should be used for which scenarios.

Right now it's not immediately obvious when I should use LocalWorkspace.create vs LocalWorkspace.createStack/createOrSelectStack/selectStack vs. Stack.create/createOrSelect/select, without looking at example usage or reading the doc comments. If LocalWorkspace.createStack/createOrSelectStack/selectStack are the mainline methods to use, wondering if we should rename create to a name that's a little less enticing. Also when I see LocalWorkspace.create I wonder whether it's creating the workspace on disk or just opening an existing directory.

I'm fine shipping an initial alpha version of the APIs as they are now, but would like to noodle on this some more to see if we can come up with more intuitive entry points into this API.

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 do think that the answer here is documentation.

Happy to revisit this later if we come up with better ideas.

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.

None yet

3 participants