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

Adds remote .zip archive support to pulumi new #14443

Merged
merged 3 commits into from Oct 31, 2023
Merged

Conversation

kpitzen
Copy link
Contributor

@kpitzen kpitzen commented Oct 30, 2023

Description

Fixes https://github.com/pulumi/pulumi.ai/issues/363

This change introduces a new accepted suffix to pulumi new HTTP arguments - .zip.

When a URL of the form http[s]://<url>.zip is provided to pulumi new, we send a GET request to <url> (without the .zip suffix) with the accept header set to application/zip. It is then up to whatever is hosted at that location to correctly return a binary zip archive in its response.

We then parse that response in memory and write each file to a tempdir as we do with current templates, which can then be picked up by the standard pulumi new templating logic.

Checklist

  • I have run make tidy to update any new dependencies
  • I have run make lint to verify my code passes the lint check
    • I have formatted my code using gofumpt
  • I have added tests that prove my fix is effective or that my feature works
  • I have run make changelog and committed the changelog/pending/<file> documenting my change
  • Yes, there are changes in this PR that warrants bumping the Pulumi Cloud API version

@pulumi-bot
Copy link
Contributor

pulumi-bot commented Oct 30, 2023

Changelog

[uncommitted] (2023-10-31)

Features

  • [cli/new] Adds support for remote zip archive templates to pulumi new
    #14443

@kpitzen kpitzen force-pushed the KP/AddPulumiNewAI branch 6 times, most recently from 61bb8fe to 5ce130d Compare October 30, 2023 20:04
@kpitzen kpitzen added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Oct 30, 2023
@kpitzen
Copy link
Contributor Author

kpitzen commented Oct 30, 2023

Added no-changelog-required per @AaronFriel

sdk/go/common/workspace/templates_ai.go Outdated Show resolved Hide resolved
sdk/go/common/workspace/templates_ai.go Outdated Show resolved Hide resolved
@lukehoban
Copy link
Member

My suggested approach for this feature is a bit different from what is implemented here.

  1. Add support for a general Zip format for Pulumi templates in the CLI. This is an alternative to acquiring via a git ref.
  2. Have the AI service use that to provide a dynamically built Zip file as a source.

This avoids coupling details of AI to the Pulumi new feature, and maintains more flexibility to iterate and improve on the Pulumi AI server side, while exposing a more generally useful and foundational capability in the CLI.

Thoughts?

@kpitzen
Copy link
Contributor Author

kpitzen commented Oct 31, 2023

My suggested approach for this feature is a bit different from what is implemented here.

1. Add support for a general Zip format for Pulumi templates in the CLI. This is an alternative to acquiring via a git ref.

2. Have the AI service use that to provide a dynamically built Zip file as a source.

This avoids coupling details of AI to the Pulumi new feature, and maintains more flexibility to iterate and improve on the Pulumi AI server side, while exposing a more generally useful and foundational capability in the CLI.

Thoughts?

I considered this approach initially, but wanted to avoid building an API that would be difficult to iterate upon and test - providing the data in a structured format as I've done here has made it much simpler to work on prompt engineering, etc, while (with @Frassle 's prompting) keeping the logic in the CLI as simple as possible - we just need a list of filenames and contents which the CLI writes in the same place it would expect a standard template to exist. Notably, this does not couple the CLI to "AI" other than nominally in this case. We could just as easily rename all of these concepts and consider this a "REST standard" for Pulumi templates.

I suppose we could add a layer which, rather than returning the JSON we see here, returns the same contents as an archive, but it does make the API a bit more opaque (and any contracts provided by the service more difficult to enforce). I'd personally prefer to stick to structured data where we can, but I acknowledge the value you mention as well.

@Frassle
Copy link
Member

Frassle commented Oct 31, 2023

Add support for a general Zip format for Pulumi templates in the CLI. This is an alternative to acquiring via a git ref.

This seems fine to me. Could then just allow any http://host/path.zip url.

but wanted to avoid building an API that would be difficult to iterate upon and test

I'm not sure I really follow why a zip file is any harder to test than the JSON files? A template is just a file tree, I don't think the CLI will ever accept anything more than that (given the constraints of what git gives us). A zip file seems a perfectly good representation of a file tree. Why does this make testing the AI service harder?

@kpitzen
Copy link
Contributor Author

kpitzen commented Oct 31, 2023

Add support for a general Zip format for Pulumi templates in the CLI. This is an alternative to acquiring via a git ref.

This seems fine to me. Could then just allow any http://host/path.zip url.

but wanted to avoid building an API that would be difficult to iterate upon and test

I'm not sure I really follow why a zip file is any harder to test than the JSON files? A template is just a file tree, I don't think the CLI will ever accept anything more than that (given the constraints of what git gives us). A zip file seems a perfectly good representation of a file tree. Why does this make testing the AI service harder?

It's not about the CLI when it comes to testing the service - if we're relying on the CLI for all of our iteration and testing, or worse, manually extracting an archive and checking the files each time, it's just going to be too slow. As I describe above, we can provide the capability to download a zipfile, but if we're already providing all of the required data in a way that provides some amount of structural guarantees, what's the downside? Do we have users asking to pulumi new zipfiles? I'd expect we'd have built that feature already if we had.

Anyway, I mentioned a possible middle-ground above. We can easily use an Accept header to branch off- if we're adamant that we want to use an opaque archive, we can certainly do so. We're biased towards shipping quickly per @AaronFriel 's mandate here, and this is what got us to where we are. If we want to do something different, we can, but it will slow us down.

sdk/go/common/workspace/templates_zip.go Outdated Show resolved Hide resolved
sdk/go/common/workspace/templates_zip.go Outdated Show resolved Hide resolved
sdk/go/common/workspace/templates_zip.go Outdated Show resolved Hide resolved
sdk/go/common/workspace/templates_zip.go Outdated Show resolved Hide resolved
@kpitzen kpitzen changed the title Adds ai:// scheme support to pulumi new Adds remote .zip archive support to pulumi new Oct 31, 2023
@kpitzen kpitzen force-pushed the KP/AddPulumiNewAI branch 3 times, most recently from d95a734 to d2f39fa Compare October 31, 2023 15:31
@kpitzen kpitzen requested a review from Frassle October 31, 2023 15:36
sdk/go/common/workspace/templates_zip.go Outdated Show resolved Hide resolved
sdk/go/common/workspace/templates_zip.go Outdated Show resolved Hide resolved
…zipfiles

We depend upon the user to provide a URL with a .zip suffix
in order to determine which methodology to use, and assume the API
serves this artifact on a PUT endpoint at the given path.

We also provide a clean passthrough of any query parameters, though
providing those on the command line can be a bit cumbersome.
@kpitzen kpitzen added this pull request to the merge queue Oct 31, 2023
@lukehoban
Copy link
Member

I like the simplicity of the ZIP approach here.

Two things:

  1. Can we add a description which captures what this change does?
  2. Can we add tests?

@AaronFriel AaronFriel removed this pull request from the merge queue due to a manual request Oct 31, 2023
@kpitzen
Copy link
Contributor Author

kpitzen commented Oct 31, 2023

I like the simplicity of the ZIP approach here.

Two things:

1. Can we add a description which captures what this change does?

2. Can we add tests?

Absolutely - just added a description above. Will add tests now.

@Frassle
Copy link
Member

Frassle commented Oct 31, 2023

Probably reasonable to add the changelog entry for this now as well?

@Frassle Frassle removed the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Oct 31, 2023
@kpitzen
Copy link
Contributor Author

kpitzen commented Oct 31, 2023

Probably reasonable to add the changelog entry for this now as well?

Since it's not tied to AI, I suppose we can. Do we want to consider the API contract (GET <url>.zip with the Accept header) stable?

@Frassle
Copy link
Member

Frassle commented Oct 31, 2023

Do we want to consider the API contract (GET .zip with the Accept header) stable?

Yeh that seems reasonable to me.

@kpitzen
Copy link
Contributor Author

kpitzen commented Oct 31, 2023

@justinvp ++ @Frassle re-requested reviews w/ tests + changelog entry

Copy link
Member

@AaronFriel AaronFriel left a comment

Choose a reason for hiding this comment

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

Just a question on the URL handling, the Zip test looks good though with the mock server.

sdk/go/common/workspace/templates_zip.go Outdated Show resolved Hide resolved
@kpitzen kpitzen added this pull request to the merge queue Oct 31, 2023
Merged via the queue into master with commit f82b735 Oct 31, 2023
44 checks passed
@kpitzen kpitzen deleted the KP/AddPulumiNewAI branch October 31, 2023 22:45
@justinvp justinvp mentioned this pull request Nov 2, 2023
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

6 participants