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

Dependencies of component providers aren't downloaded #10521

Closed
cnunciato opened this issue Aug 29, 2022 · 7 comments · Fixed by #10691
Closed

Dependencies of component providers aren't downloaded #10521

cnunciato opened this issue Aug 29, 2022 · 7 comments · Fixed by #10691
Assignees
Labels
area/plugins Plugin acquisition and resolution kind/bug Some behavior is incorrect or out of spec p1 A bug severe enough to be the next item assigned to an engineer resolution/fixed This issue was fixed
Milestone

Comments

@cnunciato
Copy link
Member

cnunciato commented Aug 29, 2022

Given the following YAML program, which uses the Synced Folder component (a package built from pulumi-component-provider-ts-boilerplate):

name: yaml-synced-folder-test
runtime: yaml

resources:
  bucket:
    type: aws:s3:Bucket

  bucket-folder:
    type: synced-folder:index:S3BucketFolder
    properties:
      path: ./site   # You'll need a folder with some files here.
      bucketName: ${bucket.bucket}

... the initial update fails because the provider's resource plugin wasn't downloaded:

pulumi up
Previewing update (dev)

View Live: https://app.pulumi.com/cnunciato/yaml-synced-folder-test/dev/previews/bbb32df0-7f6d-4640-97f7-ed34baa7b9d6

Downloading plugin: 38.06 MiB / 38.06 MiB [=========================] 100.00% 4s
[resource plugin synced-folder-0.0.8] installing
Downloading plugin: 126.54 MiB / 126.54 MiB [=======================] 100.00% 9s
[resource plugin aws-5.13.0] installing

     Type                                   Name                         Plan       Info
 +   pulumi:pulumi:Stack                    yaml-synced-folder-test-dev  create     
 +   ├─ aws:s3:Bucket                       bucket                       create     
 +   ├─ synced-folder:index:S3BucketFolder  bucket-folder                create     
     └─ pulumi:providers:aws                default_5_12_1                          1 error
 
Diagnostics:
  pulumi:providers:aws (default_5_12_1):
    error: no resource plugin 'pulumi-resource-aws' found in the workspace at version v5.12.1 or on your $PATH, install the plugin using `pulumi plugin install resource aws v5.12.1`

This is presumably happening because the current version of pulumi-aws is 5.13 (which does get downloaded), whereas the latest synced-folder was built against pulumi-aws@5.12.1. The following is a snippet from my local yarn.lock that shows this:

"@pulumi/aws@^5.0.0":
  version "5.12.1"
  resolved "https://registry.yarnpkg.com/@pulumi/aws/-/aws-5.12.1.tgz#6fdae06c825e852392fc63f4dbe835626acbe36f"
  integrity sha512-QRuqXgNnDNWQZu0xcGHhbgLLF+dYyd9wt2dfsIy5UeP8nExaWmWv0DS4AhWO9KaaUGH+ifqsmX7RdfAzDmVBPw==
  dependencies:
    "@pulumi/pulumi" "^3.0.0"
    aws-sdk "^2.0.0"
    builtin-modules "3.0.0"
    mime "^2.0.0"
    read-package-tree "^5.2.1"
    resolve "^1.7.1"

To repro, ensure you don't have 5.12.1 installed already, then run pulumi up. I would expect Pulumi to download 5.12.1 and for the update to work without requiring an additional manual install.

@cnunciato cnunciato added kind/enhancement Improvements or new features needs-triage Needs attention from the triage team kind/bug Some behavior is incorrect or out of spec and removed kind/enhancement Improvements or new features labels Aug 29, 2022
@t0yv0 t0yv0 removed the needs-triage Needs attention from the triage team label Aug 29, 2022
@t0yv0 t0yv0 added this to the 0.78 milestone Aug 29, 2022
@t0yv0
Copy link
Member

t0yv0 commented Aug 29, 2022

CC @AaronFriel

Logging discussion with Friel we had async on this. It would be better UX to kick off automatic plugin installation at the point where we fail here, fixing the primary user issue. IN the long term we may need to think about Pulumi lock files and dep management tools like pulumi why. The runtime dependency (hidden) of a component implementation here needs a different AWS Provider version from the SDK used in the program, and while that should be OK it can be surprising at least in terms of dependency bloat, and having tools eventually that explain this would be great.

@mikhailshilkov mikhailshilkov added p1 A bug severe enough to be the next item assigned to an engineer area/plugins Plugin acquisition and resolution labels Aug 29, 2022
@mikhailshilkov mikhailshilkov assigned Zaid-Ajaj and unassigned t0yv0 Aug 29, 2022
@Frassle
Copy link
Member

Frassle commented Aug 30, 2022

I think this might just be #5619?

@t0yv0
Copy link
Member

t0yv0 commented Aug 30, 2022

Hmm yes! The lockfile ideas can go into the extension of #5619 ; but to be clear this particular bug is asking to extend the magic auto-install behavior to cover the case of a runtime dependency of the synced-folder provider.

@Frassle
Copy link
Member

Frassle commented Aug 30, 2022

but to be clear this particular bug is asking to extend the magic auto-install behavior to cover the case of a runtime dependency of the synced-folder provider.

I think the easiest fix here is to just download missing plugins as we hit them. That will close #5619 off, and unblock this use case, not ideally but at least not p1 bad.

We can then leave this ticket to track adding plugin lookup from providers, not just programs. Which would be good (and I like your other ideas of lockfiles, pulumi why, and no auto downloads) but that's much more work but at least won't need doing so urgently with #5619 done.

@t0yv0
Copy link
Member

t0yv0 commented Aug 30, 2022

Yes! Sorry bad protocol on my part. Did not mean to ask for increased scope. Should've logged the ideas as separate ticket.

@Frassle Frassle removed the p1 A bug severe enough to be the next item assigned to an engineer label Aug 31, 2022
@Zaid-Ajaj Zaid-Ajaj added the p1 A bug severe enough to be the next item assigned to an engineer label Sep 8, 2022
@cnunciato
Copy link
Member Author

cnunciato commented Sep 8, 2022

I believe this is fixed -- is that right? Or is there more y'all want to do?

Thank you again for fixing this!

@AaronFriel
Copy link
Member

I think this was reopened due to a revert in 3.39.3.

bors bot added a commit that referenced this issue Sep 14, 2022
10691: Bring back auto-install plugins r=Zaid-Ajaj a=Zaid-Ajaj

<!--- 
Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation.
-->

# Description

<!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. -->

This PR brings back #10530 (cc `@AaronFriel)`

Fixes #10521
Fixes #10216 

After debugging for ages, it turns out that the [package](https://github.com/checkly/pulumi-checkly/tree/main/sdk/nodejs) that caused the p0/revert of the original PRs were faulty in the sense that they didn't use latest codegen to generate the nodejs package. The generated package.json had
```json
{
    "pulumi": {
        "resource": true,
        "pluginDownloadURL": "https://github.com/checkly/pulumi-checkly/releases/download/v1.1.2"
    }
}
```
And couldn't auto install the third-party plugin because: 
 - Doesn't have a `name` which is now going to be required for third-party packages when the package name is not ``@pulumi/{something}`(see` change in pulumi-language-nodejs/main.go)
 - When `pulumi.name` is missing, `pulumi-language-nodejs/main.go` tried to "derive" the plugin name by simply removing the "`@"` sign from the package name but that is still is wrong because the plugin name was "checkly" and the derived plugin name was "checkly/pulumi" hence why a `packageJson.pulumi.name` is now required
 - Had `pluginDownloadURL` property instead of `server` which is not read anywhere by Pulumi except from the original schema.json itself, not the generated package.json file where it should be transformed to `server`
 - Third-party package used a very old version of tfgen v3.19.0 which is from February this year. 

I've confirmed that after installing the plugin via the install script added to the package _and_ adding a name to the `pulumi` section that the error no longer shows since now

## Checklist

<!--- Please provide details if the checkbox below is to be left unchecked. -->
- [ ] I have added tests that prove my fix is effective or that my feature works
<!--- 
User-facing changes require a CHANGELOG entry.
-->
- [ ] I have updated the [CHANGELOG-PENDING](https://github.com/pulumi/pulumi/blob/master/CHANGELOG_PENDING.md) file with my change
<!--
If the change(s) in this PR is a modification of an existing call to the Pulumi Service,
then the service should honor older versions of the CLI where this change would not exist.
You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version
  <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. -->


Co-authored-by: Zaid Ajaj <zaid.naom@gmail.com>
bors bot added a commit that referenced this issue Sep 14, 2022
10691: Bring back auto-install plugins r=Zaid-Ajaj a=Zaid-Ajaj

<!--- 
Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation.
-->

# Description

<!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. -->

This PR brings back #10530 (cc `@AaronFriel)`

Fixes #10521
Fixes #10216 

After debugging for ages, it turns out that the [package](https://github.com/checkly/pulumi-checkly/tree/main/sdk/nodejs) that caused the p0/revert of the original PRs were faulty in the sense that they didn't use latest codegen to generate the nodejs package. The generated package.json had
```json
{
    "pulumi": {
        "resource": true,
        "pluginDownloadURL": "https://github.com/checkly/pulumi-checkly/releases/download/v1.1.2"
    }
}
```
And couldn't auto install the third-party plugin because: 
 - Doesn't have a `name` which is now going to be required for third-party packages when the package name is not ``@pulumi/{something}`(see` change in pulumi-language-nodejs/main.go)
 - When `pulumi.name` is missing, `pulumi-language-nodejs/main.go` tried to "derive" the plugin name by simply removing the "`@"` sign from the package name but that is still is wrong because the plugin name was "checkly" and the derived plugin name was "checkly/pulumi" hence why a `packageJson.pulumi.name` is now required
 - Had `pluginDownloadURL` property instead of `server` which is not read anywhere by Pulumi except from the original schema.json itself, not the generated package.json file where it should be transformed to `server`
 - Third-party package used a very old version of tfgen v3.19.0 which is from February this year. 

I've confirmed that after installing the plugin via the install script added to the package _and_ adding a name to the `pulumi` section that the error no longer shows since now

## Checklist

<!--- Please provide details if the checkbox below is to be left unchecked. -->
- [ ] I have added tests that prove my fix is effective or that my feature works
<!--- 
User-facing changes require a CHANGELOG entry.
-->
- [ ] I have updated the [CHANGELOG-PENDING](https://github.com/pulumi/pulumi/blob/master/CHANGELOG_PENDING.md) file with my change
<!--
If the change(s) in this PR is a modification of an existing call to the Pulumi Service,
then the service should honor older versions of the CLI where this change would not exist.
You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version
  <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. -->


Co-authored-by: Zaid Ajaj <zaid.naom@gmail.com>
@bors bors bot closed this as completed in 5beffd8 Sep 14, 2022
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Sep 14, 2022
abhinav pushed a commit to pulumi/pulumi-dotnet that referenced this issue Jan 11, 2023
…umi/pulumi#10713 pulumi/pulumi#10714 pulumi/pulumi#10715 pulumi/pulumi#10716 pulumi/pulumi#10718 pulumi/pulumi#10719 pulumi/pulumi#10721 pulumi/pulumi#10722 pulumi/pulumi#10723 pulumi/pulumi#10724

10691: Bring back auto-install plugins r=Zaid-Ajaj a=Zaid-Ajaj

<!--- 
Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation.
-->

# Description

<!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. -->

This PR brings back pulumi/pulumi#10530 (cc `@AaronFriel)`

Fixes pulumi/pulumi#10521
Fixes pulumi/pulumi#10216 

After debugging for ages, it turns out that the [package](https://github.com/checkly/pulumi-checkly/tree/main/sdk/nodejs) that caused the p0/revert of the original PRs were faulty in the sense that they didn't use latest codegen to generate the nodejs package. The generated package.json had
```json
{
    "pulumi": {
        "resource": true,
        "pluginDownloadURL": "https://github.com/checkly/pulumi-checkly/releases/download/v1.1.2"
    }
}
```
And couldn't auto install the third-party plugin because: 
 - Doesn't have a `name` which is now going to be required for third-party packages when the package name is not ``@pulumi/{something}`(see` change in pulumi-language-nodejs/main.go)
 - When `pulumi.name` is missing, `pulumi-language-nodejs/main.go` tried to "derive" the plugin name by simply removing the "`@"` sign from the package name but that is still is wrong because the plugin name was "checkly" and the derived plugin name was "checkly/pulumi" hence why a `packageJson.pulumi.name` is now required
 - Had `pluginDownloadURL` property instead of `server` which is not read anywhere by Pulumi except from the original schema.json itself, not the generated package.json file where it should be transformed to `server`
 - Third-party package used a very old version of tfgen v3.19.0 which is from February this year. 

I've confirmed that after installing the plugin via the install script added to the package _and_ adding a name to the `pulumi` section that the error no longer shows since now

## Checklist

<!--- Please provide details if the checkbox below is to be left unchecked. -->
- [ ] I have added tests that prove my fix is effective or that my feature works
<!--- 
User-facing changes require a CHANGELOG entry.
-->
- [ ] I have updated the [CHANGELOG-PENDING](https://github.com/pulumi/pulumi/blob/master/CHANGELOG_PENDING.md) file with my change
<!--
If the change(s) in this PR is a modification of an existing call to the Pulumi Service,
then the service should honor older versions of the CLI where this change would not exist.
You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version
  <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. -->


10702: Prep for real 3.40.0 release r=AaronFriel a=AaronFriel

This turns the "safeties off" after validating the bors workflow and ensuring everything up to the dispatch commands succeeds.

It also fixes an error in `get-next-version` that resulted in the "vNext" PR pulumi/pulumi#10701 to calculate the wrong "next version", which was corrected by hand. (The branch name and commit history is evidence of the error.)

10704: ci: Add make changelog shortcut for creating changelog entries r=AaronFriel a=AaronFriel



10713: ci: Fix Windows flake with longer timeout r=AaronFriel a=AaronFriel

This fixes Windows tests failing like this due to very short (1 second) timeouts.

https://github.com/pulumi/pulumi/actions/runs/3054396239/jobs/4926314390


10714: ci: Reduce test duration by avoiding costly subtests in loop r=AaronFriel a=AaronFriel

The loops in this file create millions of subtests, the majority of which do very little work. This reduces the time to execute these tests to a couple seconds.

10715: ci: Remove v0.10.0 compatibility test r=AaronFriel a=AaronFriel

Removes a _very old_ and ostensibly disabled test. The condition in this test was actually incorrect:

```go
func skipIfNotNode610(t *testing.T) {
	nodeVer, err := getNodeVersion()
	if err != nil && nodeVer.Major == 6 && nodeVer.Minor == 10 {
		t.Skip("Skipping 0.10.0 compat tests, because current node version is not 6.10.X")
	}
}
```

This only skips **if** the node version is vMajor = 6 and vMinor == 10.

I discovered this test from bisecting which test was installing a very old Pulumi library and running node-gyp to link an even older gRPC library.

10716: ci: Enable async component builds, synchronous Yarn execution r=AaronFriel a=AaronFriel

The `make test_build` command is blocking for all integration tests, adding roughly 2 minutes of execution time to each. It runs a series of Node, Python, and Go CLI commands to configure providers. These build steps use a file lock to ensure only one runs at a time, and additionally parallelizes the three build steps.

This moves the setup step into an asynchronous action performed on-demand, which enables greater concurrency and allows integration tests that perform _no_ setup to skip most of the costly build process altogether.

The second commit, dependent on the first, adds a global mutex in the sdk library for running `yarn` commands. The Yarn CLI itself has a lock that prohibits concurrent execution, however it times out. On concurrent executions then, we get spurious test failures because `yarn` failed to execute waiting for a separate execution.

These changes improve test reliability and reduce integration test execution time by roughly 2 minutes across all jobs.

10718: ci: Skip refresh on tests that should fail fast r=AaronFriel a=AaronFriel

These tests performed unnecessary refresh steps. Happened to catch these while optimizing build times. The impact on execution times was negligible.

10719: ci: Use yarn link before install, ensure Pulumi latest is used r=AaronFriel a=AaronFriel

These cut execution time on myriad node tests by 5 to 30 seconds each. Linking ``@pulumi/pulumi`` first, before `yarn install`, enables yarn to skip downloading and extracting the node SDK before replacing it with a symlink.

10721: ci: Increase bors timeout r=AaronFriel a=AaronFriel

The default timeout is one hour, which our CI runs can exceed. This should give us a buffer while work is done to reduce execution time to a reasonable threshold.

10722: ci: Improve test parallelism, enabled by pulumi/pulumi#10263 r=AaronFriel a=AaronFriel

These tests no longer mutate environment variables, instead using `e.SetEnvVars` to only set env vars on child processes since pulumi/pulumi#10263.

10723: ci: Reduce test duration with parallel subtests r=AaronFriel a=AaronFriel

This change parallelizes subtests. Although there are only ~200 combinations tested, they are fully parallelizable and this reduces execution time for these tests from 80-100s to 10-15s each, reducing execution time for the package by about a minute or a little more. Each parent test is compute bound and already run in parallel, so the reduction isn't additive.

10724: ci: Enable robust retries on tests r=AaronFriel a=AaronFriel

This adds a retry script to `./scripts/retry` which takes any shell command and retries it up to `PULUMI_TEST_RETRIES` times. On each iteration of the script, parallelism and variation is reduced.


Co-authored-by: Zaid Ajaj <zaid.naom@gmail.com>
Co-authored-by: Aaron Friel <mayreply@aaronfriel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/plugins Plugin acquisition and resolution kind/bug Some behavior is incorrect or out of spec p1 A bug severe enough to be the next item assigned to an engineer resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants