Skip to content

chore: separate deploy from polywrap project#1482

Merged
dOrgJelli merged 28 commits intoorigin-devfrom
chore/separate-deploy-from-polywrap-project
Jan 18, 2023
Merged

chore: separate deploy from polywrap project#1482
dOrgJelli merged 28 commits intoorigin-devfrom
chore/separate-deploy-from-polywrap-project

Conversation

@cbrzn
Copy link
Copy Markdown
Contributor

@cbrzn cbrzn commented Jan 13, 2023

Creates a new PolywrapDeploy class which takes care of the deploy logic. Now user does not need to have polywrap.yaml to deploy a wrapper.

TODO:

  • Remove deploy attribute from extensions in polywrap.yaml

block: #1476
closes #1426

@cbrzn cbrzn force-pushed the chore/separate-deploy-from-polywrap-project branch from 76c080e to 4600b86 Compare January 13, 2023 18:07
@cbrzn cbrzn requested review from nerfZael and pileks as code owners January 14, 2023 12:36
@cbrzn cbrzn marked this pull request as draft January 14, 2023 12:36
@cbrzn cbrzn force-pushed the chore/separate-deploy-from-polywrap-project branch from af4806c to fcfaeb3 Compare January 14, 2023 12:55
@cbrzn cbrzn force-pushed the chore/separate-deploy-from-polywrap-project branch from fcfaeb3 to 4f2a3aa Compare January 14, 2023 13:25
@cbrzn cbrzn force-pushed the chore/separate-deploy-from-polywrap-project branch from 4f2a3aa to e5be4d0 Compare January 14, 2023 13:57
@cbrzn
Copy link
Copy Markdown
Contributor Author

cbrzn commented Jan 14, 2023

one open question: now that this pr removes the dependency of having a polywrap.yaml to do a deploy, would it make sense to remove deploy from the extensions attribute of the polywrap.yaml manifest?

@cbrzn cbrzn marked this pull request as ready for review January 14, 2023 14:08
@cbrzn cbrzn requested a review from krisbitney as a code owner January 14, 2023 14:08
@cbrzn cbrzn force-pushed the chore/separate-deploy-from-polywrap-project branch from 310b77f to 1235221 Compare January 14, 2023 14:30
@cbrzn cbrzn force-pushed the chore/separate-deploy-from-polywrap-project branch from 1bcfca7 to 02e7273 Compare January 15, 2023 19:38
@pileks
Copy link
Copy Markdown
Contributor

pileks commented Jan 16, 2023

one open question: now that this pr removes the dependency of having a polywrap.yaml to do a deploy, would it make sense to remove deploy from the extensions attribute of the polywrap.yaml manifest?

I think there's precedent for this (polywrap.test.yaml), and deploy is a very similar manifest to test, so I'm in 100% agreement with this.

@cbrzn
Copy link
Copy Markdown
Contributor Author

cbrzn commented Jan 16, 2023

one open question: now that this pr removes the dependency of having a polywrap.yaml to do a deploy, would it make sense to remove deploy from the extensions attribute of the polywrap.yaml manifest?

I think there's precedent for this (polywrap.test.yaml), and deploy is a very similar manifest to test, so I'm in 100% agreement with this.

@pileks perfect, working on this now

@cbrzn cbrzn requested a review from Niraj-Kamdar as a code owner January 16, 2023 21:13
Copy link
Copy Markdown
Contributor

@pileks pileks left a comment

Choose a reason for hiding this comment

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

Overall this is a great change and a step forward in a direction we've already taken with build and codegen!

One thing I'd note - When migrating the templates, it would be a good excercise to migrate them using polywrap manifest migrate, so that you make sure that you are creating a migration that can truly migrate our manifests (of course, there can always be breaking changes which prevent you from this).

Apart from some nitpicks, I'd say that I'd personally go with the delete approach in the migration.

Comment thread packages/cli/src/lib/deploy/PolywrapDeploy.ts Outdated
Comment thread packages/cli/src/lib/deploy/PolywrapDeploy.ts Outdated
Comment thread packages/cli/src/lib/deploy/PolywrapDeploy.ts Outdated
Comment thread packages/js/manifests/polywrap/src/formats/polywrap/migrators/0.3.0_to_0.4.0.ts Outdated
@cbrzn cbrzn mentioned this pull request Jan 17, 2023
pileks
pileks previously approved these changes Jan 17, 2023
Copy link
Copy Markdown
Contributor

@pileks pileks left a comment

Choose a reason for hiding this comment

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

Looking great!

@dOrgJelli
Copy link
Copy Markdown
Contributor

Hey @cbrzn @pileks, some thoughts on how I think we should treat the project manifest (polywrap.yaml) and what we currently call "extensions" manifests (build, infra, test, deploy, etc).

First thought is, I think we should remove the concept of a project "extension" and create a much simpler & uniform experience for the CLI. Instead I think we should call them command manifests, which can be defined for the various CLI commands: polywrap.build.yaml, polywrap.infra.yaml, polywrap.test.yaml, polywrap.deploy.yaml.

Second, we should have the command manifests back-link to the project they are referencing (if needed). Currently only the polywrap.build.yaml manifest needs to reference a project that is to be built. It should do this through a property within itself, for example project: ./polywrap.yaml.

Lastly, commands will have default naming conventions for their manifests (i.e. polywrap.build.yaml, polywrap.deploy.yaml, etc).

The result of doing these things is that we:

  • Create a much simpler experience that can be explained easily.
  • Remove the unnecessary concept of "project extensions", which doesn't apply well to all manifest types (as we see here with deploy).

Thoughts?

Comment thread packages/js/manifests/polywrap/src/formats/polywrap/index.ts Outdated
@dOrgJelli
Copy link
Copy Markdown
Contributor

Okay, spent some time digging into this #1482 (comment) and have a bit more of an appreciation for the concept of a "project exetension". I think project extensions make sense for things like the build & codegen command, since they both require a project manifest (polywrap.yaml), and can optionally have additional configuration options set within another manifest (polywrap.build.yaml), which is the extension manifest.

So, this means that the following will be true:

  • Project commands like polywrap build and polywrap codegen will require a polywrap.yaml project manifest, and will look for an optional extension manifest for added configuration options. For example: polywrap.build.yaml, polywrap.codegen.yaml.
  • Stand-alone commands like polywrap infra, polywrap test, and polywrap deploy will not require a polywrap.yaml manifest be present, and will instead look for their command manifests. For example: polywrap.infra.yaml, polywrap.test.yaml, polywrap.deploy.yaml.

I know this is right back where we started, but just wanted to fully explore the options here and explain the rationale.

@dOrgJelli
Copy link
Copy Markdown
Contributor

Here's the PR @cbrzn: #1486

@cbrzn
Copy link
Copy Markdown
Contributor Author

cbrzn commented Jan 18, 2023

I know this is right back where we started, but just wanted to fully explore the options here and explain the rationale.

@dOrgJelli what you said makes sense, and I also thought that we should only have build as an extension in polywrap.yaml. Thanks for explaining the thing about the version, nice catch there.

I will merge and fix the tests

Comment thread packages/cli/src/lib/deploy/Deployer.ts
Comment thread packages/cli/src/lib/deploy/Deployer.ts
dOrgJelli
dOrgJelli previously approved these changes Jan 18, 2023
@dOrgJelli dOrgJelli merged commit 7c2ec98 into origin-dev Jan 18, 2023
@dOrgJelli dOrgJelli deleted the chore/separate-deploy-from-polywrap-project branch April 10, 2023 16: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.

Able To Run polywrap deploy w/o polywrap.yaml Manifest

5 participants