Emit schema.Package.Version when possible #7938

iwahbe · 2021-09-09T21:22:13Z

Description

We currently offload adding the Package.Version information to makefiles in our SDKs. This means that package information which is specified in a schema.json is ignored in favor of another process. This PR has version information emitted if and only if Package.Version is supplied. This means

version.txt for dotnet
setup.py accurately contains a version for python
package.json accurately reports it's version for nodejs.

If Package.Version is not supplied, no behavior is changed.

This has 2 major motivations:

Using the schema version field should populate to the project as expected. Having the version set to "0.0.0" is not expected behavior.
We should attempt to emit valid SDKs when possible. Running dotnet build requires version.txt to exist. This prevents special cases from developing SDKs and testing our SDK codegen.

Pending Questions

How to handle the distinction python makes between VERSION and PLUGIN_VERSION

Checklist

I have added tests that prove my fix is effective or that my feature works

I have updated the CHANGELOG-PENDING file with my change

Yes, there are changes in this PR that warrants bumping the Pulumi Service API version
Decide if we should use npm or yarn for testing (discussed for Codegen testing upgrades #7989).

github-actions · 2021-09-09T21:25:34Z

Diff for pulumi-random with merge commit 5fe49eb

github-actions · 2021-09-09T21:25:53Z

Diff for pulumi-azuread with merge commit 5fe49eb

github-actions · 2021-09-09T21:28:00Z

Diff for pulumi-kubernetes with merge commit 5fe49eb

github-actions · 2021-09-09T21:28:57Z

Diff for pulumi-random with merge commit b316eca

github-actions · 2021-09-09T21:29:42Z

Diff for pulumi-azuread with merge commit b316eca

github-actions · 2021-09-09T21:32:52Z

Diff for pulumi-kubernetes with merge commit b316eca

github-actions · 2021-09-09T21:33:14Z

Diff for pulumi-gcp with merge commit 5fe49eb

github-actions · 2021-09-09T21:34:38Z

Diff for pulumi-aws with merge commit 5fe49eb

github-actions · 2021-09-09T21:38:16Z

Diff for pulumi-gcp with merge commit b316eca

github-actions · 2021-09-09T21:40:37Z

Diff for pulumi-aws with merge commit b316eca

github-actions · 2021-09-09T21:41:21Z

Diff for pulumi-azure with merge commit 5fe49eb

github-actions · 2021-09-09T21:46:44Z

Diff for pulumi-azure with merge commit b316eca

pkg/codegen/dotnet/gen.go

pgavlin · 2021-09-10T17:18:51Z

pkg/codegen/python/gen.go

+	// TODO: puipyVersion is the pythonic version of pkgVersion. The code to
+	// transform it is in pulumi/pulumictl. Should that be extracted to
+	// somewhere like pulumi/pulumi/sdk/python.go?


Yes, it does seem like we should extract that code and put it somewhere common. Not sure if pulumi/pulumi/sdk is the right place for that, but I also don't have a better suggestion offhand :) @komalali @t0yv0 ?

No strong opinions.. looks like pulumictl depends on pkg https://github.com/pulumi/pulumictl/blob/master/go.mod#L11 so if the least common ancestor is pkg we can put it there; alternatively factor into its own little dedicated GH repo and package. The barriers to that are pleasantly low. If it's really small then "a little dulication is better than a little dependency" applies here too so we can inline a copy.

I just duplicated the code.
Taking a closer look, most of the code dealt with the current git tree being dirty, and generating a semver version. Since we are provided a semver version already, I just help with the pythonic parts. It's not long. I located it in pulumi/pkg/codegen/python/utilities.go.

t0yv0 · 2021-09-13T14:53:50Z

SO I dug around how these versions are used and filed #7955 with questions. The fact Python (and Node?) auto-install Go plugins binaries in their setup.py strikes me as a bit of an unfortunate choice.

Presumably this is the distinction:

How to handle the distinction python makes between VERSION and PLUGIN_VERSION

The PLUGIN_VERSION determines which Go plugin to auto-install.

t0yv0 · 2021-09-13T14:56:16Z

I like where this is going since Makefile-based manipulations of VERSION were giving me some headache. Before we ship this it would be good for @mikhailshilkov to make sure this causes no extra problems for provider builds.

mikhailshilkov · 2021-09-13T15:54:03Z

The reason we put this version placeholder initially was to avoid version-related diffs on every commit. Has this been mitigated somehow? Adding @stack72 to chime in too.

iwahbe · 2021-09-13T16:25:52Z

The reason we put this version placeholder initially was to avoid version-related diffs on every commit. Has this been mitigated somehow? Adding @stack72 to chime in too.

@mikhailshilkov The mitigation is that if you don't specify version in schema.json, we will still generate a placeholder "version": "${VERSION}" in package.json. This change has no effect unless a version is supplied at generation time.

mikhailshilkov · 2021-09-13T17:17:41Z

@iwahbe Thank you for the clarification! Do I understand correctly that this is mostly to clean up the tests but likely won't affect the providers?

iwahbe · 2021-09-13T17:48:28Z

@iwahbe Thank you for the clarification! Do I understand correctly that this is mostly to clean up the tests but likely won't affect the providers?

It is primarily about cleaning up tests and internal consistency. It should also help decrease the amount of makefile necessary for multi-lang.

infin8x · 2021-09-14T15:26:25Z

This looks great! We'd talked about doing something similar in #7589 - am I understanding correctly that this PR achieves the "set the version in a single location" part of that issue?

iwahbe · 2021-09-14T16:58:10Z

This looks great! We'd talked about doing something similar in #7589 - am I understanding correctly that this PR achieves the "set the version in a single location" part of that issue?

@infin8x Yes. This has you set the version in the schema, and it propagates it everywhere.

github-actions · 2021-09-14T21:35:09Z

Diff for pulumi-kubernetes with merge commit fc7ccce

github-actions · 2021-09-14T21:41:54Z

Diff for pulumi-gcp with merge commit fc7ccce

github-actions · 2021-09-14T21:42:42Z

Diff for pulumi-aws with merge commit fc7ccce

github-actions · 2021-09-14T21:51:54Z

Diff for pulumi-azure with merge commit fc7ccce

pgavlin · 2021-09-15T15:54:45Z

This doesn't work well for testing, since version.Version changes with each commit. I can think of several solutions, none of which I love.

Note that this is only the case for workflows that do this explicitly by using ldflags to set version.Version. These workflows are likely to be similar to our own gitops-style workflow where the schema version will be unset. It feels like if the schema version is set, then it should align with version.Version and we should be able to emit a real value for PLUGIN_VERSION.

t0yv0 · 2021-09-15T15:56:08Z

@iwahbe I'm not really super sure, how does our system do it in practice?

I peeked at https://github.com/pulumi/pulumi-aws/blob/master/sdk/python/setup.py#L11 and I see 0.0.0. That seems to work?

Also in my investigation #7955 it turns out that Node JS packages are the other kind of packages that auto-install the plugin, yet they have no PULUMI_VERSION distinction? How do they do it? I can't seem to find the mechanism of how we hook into npm install.

iwahbe · 2021-09-16T01:23:31Z

I peeked at https://github.com/pulumi/pulumi-aws/blob/master/sdk/python/setup.py#L11 and I see 0.0.0. That seems to work?

Taking a look around pulumi-aws I saw what you did, but when ran make build in the aws repo, a setup.py file was generated with VERSION = "4.20.0" and PLUGIN_VERSION = "4.20.0". That gets setup here.

iwahbe · 2021-09-16T01:30:52Z

At this point I'm fairly convinced that the correct behavior (when not testing) is to create a pythonic version of version.Version and stick it in PLUGIN_VERSION. This accommodates our current setup, where we mirror the package version with the pypi release version, only changing it to accommodate the idiosyncrasies of pypi. This is what the current commit does.

When testing, we emit "0.0.0", because we don't want to see diffs on our commit hashes in codegen/internal/tests. Also, version.Version == "" under test.

pgavlin · 2021-09-16T21:16:01Z

pkg/codegen/python/gen.go

+	pluginVersion := "0.0.0"
+	if version.Version != "" {
+		// This happens in test builds
+		pluginVersion = version.Version
+	}


I don't think that we want to use version.Version here: "github.com/pulumi/pulumi/pkg/v3/version" is the version of the Pulumi SDK itself, and is unrelated to the schema version. Should this be

Suggested change

pluginVersion := "0.0.0"

if version.Version != "" {

// This happens in test builds

pluginVersion = version.Version

}

pluginVersion := "0.0.0"

if pkg.Version != nil {

pluginVersion = pkg.Version.String()

}

Your right. I got the version of the pulumi-resource-* confused with the version of pulumi itself.

iwahbe · 2021-09-17T21:06:57Z

@t0yv0 I want to make sure the tests pass, then I'll merge.

github-actions · 2021-09-17T22:22:42Z

Diff for pulumi-azuread with merge commit b716553

github-actions · 2021-09-17T22:32:52Z

Diff for pulumi-aws with merge commit b716553

github-actions · 2021-09-17T22:40:41Z

Diff for pulumi-azure with merge commit b716553

github-actions · 2021-09-17T22:40:55Z

Diff for pulumi-gcp with merge commit b716553

github-actions · 2021-09-17T22:45:27Z

Diff for pulumi-random with merge commit b716553

github-actions · 2021-09-17T22:47:54Z

Diff for pulumi-kubernetes with merge commit b716553

This reverts commit 906dce7.

* Revert "Emit schema.Package.Version when possible (#7938)" This reverts commit 906dce7. * Remove dotnet SDK compile checks * Add to changelog

Emit schema.Package.Version when possible

be2dad7

iwahbe self-assigned this Sep 9, 2021

iwahbe marked this pull request as draft September 9, 2021 21:22

iwahbe added the area/codegen SDK-gen, program-gen, convert label Sep 9, 2021

Update CHANGELOG_PENDING.md

d49fd02

iwahbe marked this pull request as ready for review September 10, 2021 16:31

iwahbe requested review from infin8x and pgavlin September 10, 2021 16:33

pgavlin reviewed Sep 10, 2021

View reviewed changes

t0yv0 self-requested a review September 13, 2021 14:53

iwahbe requested a review from pgavlin September 16, 2021 01:34

pgavlin reviewed Sep 16, 2021

View reviewed changes

t0yv0 mentioned this pull request Sep 17, 2021

Codegen testing upgrades #7989

Merged

8 tasks

iwahbe added 2 commits September 17, 2021 13:56

Update PLUGIN_VERSION to the package version

7a79ec5

Merge branch 'master' into iwahbe/add-version-on-package-generation

e74dd6a

t0yv0 approved these changes Sep 17, 2021

View reviewed changes

Modify tests to conform with master merge

8288298

iwahbe merged commit 906dce7 into master Sep 18, 2021

pulumi-bot deleted the iwahbe/add-version-on-package-generation branch September 18, 2021 06:11

emiliza added kind/engineering Work that is not visible to an external user size/S Estimated effort to complete (1-2 days). labels Sep 21, 2021

emiliza added this to the 0.62 milestone Sep 21, 2021

komalali mentioned this pull request Sep 22, 2021

Emitted SDK versions breaking provider builds #8033

Closed

iwahbe added a commit that referenced this pull request Sep 23, 2021

Revert "Emit schema.Package.Version when possible (#7938)"

c4488d9

This reverts commit 906dce7.

iwahbe added a commit that referenced this pull request Sep 23, 2021

Revert "Emit schema.Package.Version when possible (#7938)" (#8035)

c9b719c

* Revert "Emit schema.Package.Version when possible (#7938)" This reverts commit 906dce7. * Remove dotnet SDK compile checks * Add to changelog

Emit schema.Package.Version when possible #7938

Emit schema.Package.Version when possible #7938

Conversation

iwahbe commented Sep 9, 2021 • edited Loading

Description

Pending Questions

Checklist

github-actions bot commented Sep 9, 2021

github-actions bot commented Sep 9, 2021

github-actions bot commented Sep 9, 2021

github-actions bot commented Sep 9, 2021

github-actions bot commented Sep 9, 2021

github-actions bot commented Sep 9, 2021

github-actions bot commented Sep 9, 2021

github-actions bot commented Sep 9, 2021

github-actions bot commented Sep 9, 2021

github-actions bot commented Sep 9, 2021

github-actions bot commented Sep 9, 2021

github-actions bot commented Sep 9, 2021

pgavlin Sep 10, 2021

Choose a reason for hiding this comment

t0yv0 Sep 13, 2021

Choose a reason for hiding this comment

iwahbe Sep 16, 2021

Choose a reason for hiding this comment

t0yv0 commented Sep 13, 2021

t0yv0 commented Sep 13, 2021

mikhailshilkov commented Sep 13, 2021

iwahbe commented Sep 13, 2021 • edited Loading

mikhailshilkov commented Sep 13, 2021

iwahbe commented Sep 13, 2021

infin8x commented Sep 14, 2021

iwahbe commented Sep 14, 2021

github-actions bot commented Sep 14, 2021

github-actions bot commented Sep 14, 2021

github-actions bot commented Sep 14, 2021

github-actions bot commented Sep 14, 2021

pgavlin commented Sep 15, 2021

t0yv0 commented Sep 15, 2021

iwahbe commented Sep 16, 2021

iwahbe commented Sep 16, 2021

pgavlin Sep 16, 2021

Choose a reason for hiding this comment

iwahbe Sep 17, 2021

Choose a reason for hiding this comment

iwahbe commented Sep 17, 2021

github-actions bot commented Sep 17, 2021

github-actions bot commented Sep 17, 2021

github-actions bot commented Sep 17, 2021

github-actions bot commented Sep 17, 2021

github-actions bot commented Sep 17, 2021

github-actions bot commented Sep 17, 2021

iwahbe commented Sep 9, 2021 •

edited

Loading

iwahbe commented Sep 13, 2021 •

edited

Loading