Iwahbe/2715/add pulumi about command #7817
Conversation
This will make it easier to submit debug information to those who work on pulumi.
Now it's time for a design review.
Change copywrite dates.
This allows us to give the output in json.
…/2715/add-pulumi-about-command
There was a problem hiding this comment.
Some comments/questions, but otherwise LGTM!
For future PRs, it'd be nice to show some example output of running the command in the PR description.
It might be fun to add some color to the output when in interactive mode, but we can always do that later.
I ran pulumi about -j in an empty dir and got this:
$ pulumi about -j
{
"plugins": null,
"host": {
"os": "darwin",
"version": "11.4",
"arch": "x86_64"
},
"backend": {
"name": "pulumi.com",
"url": "https://app.pulumi.com/justinvp",
"user": "justinvp"
},
"currentStack": null,
"cliAbout": {
"version": "3.11.0-alpha.1629745492+784c661c",
"goVersion": "go1.17",
"goCompiler": "gc"
},
"runtime": null,
"errors": [
{},
{},
{}
]
}Two questions:
- Should we omit empty/nil values from the JSON output (e.g. "plugins", "currentStack", "runtime") via
omitemptyin thejsonstruct tags? - What's up with the errors array showing three empty objects? Should these just be strings?
The non-JSON output is:
$ pulumi about
CLI
Version 3.11.0-alpha.1629745492+784c661c
Go Version go1.17
Go Compiler gc
Host
OS darwin
Version 11.4
Arch x86_64
Backend
Name pulumi.com
URL https://app.pulumi.com/justinvp
User justinvp
Pulumi locates its logs in /var/folders/4q/8cmjgthn18x9gwgq4fvf4q7w0000gn/T/ by default
warning: Failed to get information about the plugin: no Pulumi.yaml project file found (searching upwards from /Users/justin/Desktop/2021/08/23/empty). If you have not created a project yet, use `pulumi new` to do so
warning: Failed to read project: no Pulumi.yaml project file found (searching upwards from /Users/justin/Desktop/2021/08/23/empty). If you have not created a project yet, use `pulumi new` to do so
warning: Failed to get information about the current stack: no Pulumi.yaml project file found (searching upwards from /Users/justin/Desktop/2021/08/23/empty). If you have not created a project yet, use `pulumi new` to do so
pkg/cmd/pulumi/about.go
Outdated
| var depInfo string | ||
| switch language { | ||
| case langNodejs: | ||
| depInfo = "package.json" |
There was a problem hiding this comment.
This is fine, but it's often useful to see what was actually installed by looking at package-lock.json or yarn.lock because package.json often includes version ranges. Should we mention those files?
Alternatively, we could consider showing a command to run (similar to what's currently being done for .NET) like npm ls or yarn list, or just run those commands on behalf of the user.
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestCLI(t *testing.T) { |
There was a problem hiding this comment.
Should we add some integration tests (i.e. add a new tests/about_test.go file similar to the files for the config, stack, history, etc. commands) that actually runs pulumi about and compares some of its output to expected values? (That's what I thought you meant when you were asking me about the tests).
|
I assumed that I'm open to discussion on this decision. |
| func TestBackend(t *testing.T) { | ||
| stats, err := host.Info() | ||
| if err != nil { | ||
| t.Skipf("Underlying stats call failed: %s", err) |
There was a problem hiding this comment.
When can this fail? Should we assert.NoError(t, err) instead?
There was a problem hiding this comment.
It fails on architectures that Pulumi probably isn't used on. The library I'm using to be generic over platform doesn't support all os/arch pairs that Go supports (less common BSD variations for example).
pkg/cmd/pulumi/about.go
Outdated
| binDir = "Scripts" | ||
| } | ||
| // venv makes the link to "python", so this works on windows and mac. | ||
| ex := path.Join(rootDir, "venv", binDir, "python") |
There was a problem hiding this comment.
filepath.Join (applies throughout)
There was a problem hiding this comment.
I just looked up the difference. Is is ever correct to use path.Join? I assumed (incorrectly) that the reason it existed was to be language agnostic.
pkg/cmd/pulumi/about.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| cmd := exec.Command(ex, "list", "package") |
There was a problem hiding this comment.
I wonder if we should include --include-transitive like we do in the runtime host to get the full set of dependencies?
There was a problem hiding this comment.
I intentionally printed only direct dependencies. I thought it made the input too hard to parse otherwise. I can add a flag to include transitive dependencies.
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestAboutCommands(t *testing.T) { |
There was a problem hiding this comment.
Can we add basic sanity tests for each language?
These include: 1. Make transitive dependencies optional. 2. Use filepath instead of path. 3. Correctly consider the python virtual enviroments.
Failures unique to windows require a more informative error message to diagnose.
pkg/cmd/pulumi/about_test.go
Outdated
| // Copyright 2016-2021, Pulumi Corporation. | ||
| // | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); |
There was a problem hiding this comment.
Nit: There's still an extra line in here:
| // Copyright 2016-2021, Pulumi Corporation. | |
| // | |
| // | |
| // Licensed under the Apache License, Version 2.0 (the "License"); | |
| // Copyright 2016-2021, Pulumi Corporation. | |
| // | |
| // Licensed under the Apache License, Version 2.0 (the "License"); |
Description
Adds a new command:
pulumi aboutwhich should print out the information necessary to debug a failure in the pulumi CLIFixes #2715
Checklist