[automation/go] - Implement min version checking #6577

komalali · 2021-03-18T19:46:12Z

Changes

Added a LocalWorkspace.PulumiVersion() method that returns the underlying Pulumi CLI version.
NewLocalWorkspace() now checks the underlying pulumi version during creation and stores it on the LocalWorkspace struct.
NewLocalWorkspace() checks that the PulumiVersion >= the minimum version set in minimum_version.go and returns an error to update the CLI if the check fails.

Go part of #6348 and #6419

sdk/go/x/auto/local_workspace.go

This reverts commit b963953.

stack72 · 2021-03-22T11:33:46Z

sdk/go/x/auto/minimum_version.go

+
+import "github.com/blang/semver"
+
+var minimumVersion = semver.Version{


IS there anything we are going to need to do in our 3.0 work relating to this? Is this something we need to bump on each release?

Is there anything we are going to need to do in our 3.0 work relating to this?

Yes, for the 3.0 release we will need to bump this to 3.0.0.

Is this something we need to bump on each release?

No, only when there is a change to the automation SDK that is dependent on a CLI change.

Yes, for the 3.0 release we will need to bump this to 3.0.0.

Out of curiosity - is this true? Will 3.0 of automation API require that a 3.0 CLI is installed? It's not immediately obvious it would have to?

Related - is it a breaking change to increase this? I assume so - which will make us want to do this very infrequently.

Yes. We made breaking changes to the CLI in 3.0 (standardizing select behavior) that we use under the hood in Automation API, so we will need to bump the minimum version.

stack72

@komalali this is great - a nice non-intrusive way to calculate this that only hits on the workspace creation. I am really worried about the version being embedded in this - it seems we need to do that for all of our code which means we need to remember to bump it in 4 places if we need to change it

I am not sure how much effort we want to put into this feature at this point but this feels like something we need to give some thought about

EvanBoyle · 2021-03-22T16:15:36Z

which means we need to remember to bump it in 4 places if we need to change it

This problem is kinda inherent to any feature and/or bug fix made to automation API. Just part of the game, unfortunately.

stack72 · 2021-03-22T16:30:05Z

which means we need to remember to bump it in 4 places if we need to change it

This problem is kinda inherent to any feature and/or bug fix made to automation API. Just part of the game, unfortunately.

Totally understand, it's definitely why I still hit approve as it's something we just need to be aware of rather than blocking on it

But you are 100% correct here

lukehoban · 2021-03-23T01:59:45Z

sdk/go/x/auto/local_workspace.go

@@ -479,6 +487,32 @@ func (l *LocalWorkspace) ImportStack(ctx context.Context, stackName string, stat
 	return nil
 }

+func (l *LocalWorkspace) getPulumiVersion(ctx context.Context) (semver.Version, error) {
+	stdout, stderr, errCode, err := l.runPulumiCmdSync(ctx, "version")


Perhaps a small thing - but since we already have some issues with automation api performance due to many invocations of the CLI - curious on two things:

What is the typical performance impact of making this call?

Should we make this call asynchronously, and only block when a user (directly or indirectly via some other api call that internally checks the version) requests the version?

(Same idea of course applies in all languages)

Should we make this call asynchronously, and only block when a user (directly or indirectly via some other api call that internally checks the version) requests the version?

@lukehoban the design choice here for ensuring that we have the minimum version of the CLI installed (as a means for addressing #6419) is that we will validate the pulumi version against the minimum valid version at Workspace creation. We chose to only make this call once so we could then store it on the Workspace object and avoid redoing that work later.

Typical performance impact would vary by language but a benchmark test of Workspace creation in Go result in 0.07434 ns so I feel pretty comfortable taking that hit :)

Great to see the perf impact is so small!

lukehoban · 2021-03-23T02:05:27Z

sdk/go/x/auto/minimum_version.go

+
+import "github.com/blang/semver"
+
+var minimumVersion = semver.Version{


Yes, for the 3.0 release we will need to bump this to 3.0.0.

Out of curiosity - is this true? Will 3.0 of automation API require that a 3.0 CLI is installed? It's not immediately obvious it would have to?

Related - is it a breaking change to increase this? I assume so - which will make us want to do this very infrequently.

komalali force-pushed the komalali/go-min-version branch 3 times, most recently from d80d22e to e58e2da Compare March 18, 2021 21:01

This was referenced Mar 18, 2021

[automation] - Automation API fails with CLI error if CLI version is out of date #6419

Closed

Addition of a function in the automation API to expose the underlying CLI version #6348

Closed

[automation/*] - Add version command #6575

Closed

komalali marked this pull request as ready for review March 18, 2021 21:12

komalali requested review from EvanBoyle and stack72 March 18, 2021 23:14

komalali force-pushed the komalali/go-min-version branch 3 times, most recently from 653c306 to 234a98b Compare March 19, 2021 03:17

komalali mentioned this pull request Mar 19, 2021

[automation/nodejs] - Implement min version checking #6580

Merged

justinvp reviewed Mar 19, 2021

View reviewed changes

sdk/go/x/auto/local_workspace.go Outdated Show resolved Hide resolved

komalali force-pushed the komalali/go-min-version branch 2 times, most recently from c7c4447 to 85fc2b6 Compare March 19, 2021 20:24

komalali requested a review from justinvp March 19, 2021 20:45

komalali added 8 commits March 19, 2021 19:25

Implement min version checking for go

d9ce14c

Fix min version logic and add tests

430e607

update changelog

733155a

Fix version parsing

dde95da

Revert "Fix version parsing"

33123f7

This reverts commit b963953.

Fix version parsing

4eca5fa

PR feedback

18b336d

Delete version parsing tests

bb99a94

komalali force-pushed the komalali/go-min-version branch from 85fc2b6 to 79566f1 Compare March 20, 2021 02:25

Delete Version struct

fae4c80

komalali force-pushed the komalali/go-min-version branch from 79566f1 to fae4c80 Compare March 20, 2021 02:27

This was referenced Mar 20, 2021

[automation/python] - Implement min version checking #6589

Merged

[automation/dotnet] - Implement min version checking #6590

Merged

stack72 reviewed Mar 22, 2021

View reviewed changes

stack72 approved these changes Mar 22, 2021

View reviewed changes

lukehoban reviewed Mar 23, 2021

View reviewed changes

komalali force-pushed the komalali/go-min-version branch from d66040f to 992f780 Compare March 23, 2021 04:10

Fix major version mismatch error

1ecab58

komalali force-pushed the komalali/go-min-version branch from 992f780 to 1ecab58 Compare March 23, 2021 04:20

komalali merged commit 2fde296 into master Mar 23, 2021

komalali deleted the komalali/go-min-version branch March 23, 2021 06:04

EvanBoyle mentioned this pull request Oct 18, 2023

[auto] Allow accessing current user organization membership and current backend URL via API #10356

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[automation/go] - Implement min version checking #6577

[automation/go] - Implement min version checking #6577

komalali commented Mar 18, 2021 •

edited

Loading

stack72 Mar 22, 2021

komalali Mar 23, 2021

lukehoban Mar 23, 2021

komalali Mar 23, 2021

stack72 left a comment

EvanBoyle commented Mar 22, 2021

stack72 commented Mar 22, 2021

lukehoban Mar 23, 2021

lukehoban Mar 23, 2021

komalali Mar 23, 2021

lukehoban Mar 23, 2021

lukehoban Mar 23, 2021


		import "github.com/blang/semver"

		var minimumVersion = semver.Version{

[automation/go] - Implement min version checking #6577

[automation/go] - Implement min version checking #6577

Conversation

komalali commented Mar 18, 2021 • edited Loading

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stack72 left a comment

Choose a reason for hiding this comment

EvanBoyle commented Mar 22, 2021

stack72 commented Mar 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komalali commented Mar 18, 2021 •

edited

Loading