-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Generate scripts/install-pulumi-plugin.json #8730
Conversation
Diff for pulumi-random with merge commit 31c71d7 |
Diff for pulumi-azuread with merge commit 31c71d7 |
Diff for pulumi-kubernetes with merge commit 31c71d7 |
Diff for pulumi-gcp with merge commit 31c71d7 |
Diff for pulumi-azure with merge commit 31c71d7 |
Diff for pulumi-aws with merge commit 31c71d7 |
Codecov Report
@@ Coverage Diff @@
## master #8730 +/- ##
==========================================
+ Coverage 59.33% 59.36% +0.03%
==========================================
Files 637 637
Lines 97740 97800 +60
Branches 1385 1385
==========================================
+ Hits 57995 58060 +65
+ Misses 36470 36462 -8
- Partials 3275 3278 +3
Continue to review full report at Codecov.
|
Diff for pulumi-azure-native with merge commit 31c71d7 |
// Ideally, this `scripts` section would include an install script that installs the provider, however, doing | ||
// so causes problems when we try to restore package dependencies, since we must do an install for that. So | ||
// we have another process that adds the install script when generating the package.json that we actually | ||
// publish. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm. Is it a real problem in practice? Generally, we yarn link
a locally built package. Will the install
script run as part of a yarn link
and try to install a plugin that hasn't been released yet?
(Wondering the same if we switch off yarn
to using npm link
, i.e. #8003)
If this is a problem, we may have to rethink this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have built a project linked against a code generated module (using yarn link
). It works without running the install
script. npm link
does run the install
script. We can mitigate this issue by having the install script exit cleanly if it is passed a version of ${VERSION}
, since we don't update that until the version is known (and the package is published).
Implemented as 3e3e916
Diff for pulumi-azuread with merge commit 18a99ba |
Diff for pulumi-random with merge commit 18a99ba |
Diff for pulumi-kubernetes with merge commit 18a99ba |
Diff for pulumi-gcp with merge commit 18a99ba |
Diff for pulumi-azure with merge commit 18a99ba |
Diff for pulumi-aws with merge commit 18a99ba |
Diff for pulumi-azure-native with merge commit 18a99ba |
Diff for pulumi-azuread with merge commit 184fb28 |
Diff for pulumi-random with merge commit 184fb28 |
Diff for pulumi-kubernetes with merge commit 184fb28 |
Diff for pulumi-gcp with merge commit 184fb28 |
Diff for pulumi-azure with merge commit 184fb28 |
Diff for pulumi-aws with merge commit 184fb28 |
Diff for pulumi-azure-native with merge commit 184fb28 |
Diff for pulumi-random with merge commit a72a5cd |
Diff for pulumi-azuread with merge commit a72a5cd |
Diff for pulumi-kubernetes with merge commit a72a5cd |
Diff for pulumi-gcp with merge commit a72a5cd |
Diff for pulumi-azure with merge commit a72a5cd |
Diff for pulumi-aws with merge commit a72a5cd |
Diff for pulumi-azure-native with merge commit a72a5cd |
pkg/codegen/internal/test/testdata/cyclic-types/nodejs/package.json
Outdated
Show resolved
Hide resolved
pkg/codegen/nodejs/gen.go
Outdated
// NOTE: version split is intended to prevent sed style replacement | ||
if (args.indexOf("${VERS" + "ION}") != -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clever. Can we get away without it, though, and just look for the "${VERSION}"
string as-is? I don't think we're doing sed style replacement in this file anywhere. And if there is a pluginDownloadURL
set, the URL could contain ${VERSION}
with #8507, so it's possible this file may end up having ${VERSION}
in it anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that #8507 ensures that you are right here. Good catch.
Diff for pulumi-random with merge commit ba6c766 |
Diff for pulumi-kubernetes with merge commit ba6c766 |
Diff for pulumi-azuread with merge commit ba6c766 |
Diff for pulumi-gcp with merge commit ba6c766 |
Diff for pulumi-azure with merge commit ba6c766 |
Diff for pulumi-aws with merge commit ba6c766 |
Diff for pulumi-azure-native with merge commit ba6c766 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@stack72, I am pretty sure that there's no need to immediately update the publish-tfgen-package
script as this codegen change is adopted in our provider repos.
From what I can tell and from trying out the script locally, the existing script will continue to run and overwrite what's been generated from this change (which is fine). https://github.com/pulumi/scripts/blob/b6ee1692954d2b39e7a22b0db80b8cc4b735c101/ci/publish-tfgen-package#L33-L42
Eventually, when all the provider repos have been updated, we can remove those lines from publish-tfgen-package
so that it'll just use what is generated.
Description
Fixes #8701
Checklist