Warn about ambient plugins loaded from $PATH #13607
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Changelog[uncommitted] (2023-07-27)Features
|
thomas11
approved these changes
Jul 27, 2023
| // We prefer the ambient path, but we need to check if this is the same as the bundled | ||
| // path to decide if we're warning or not. | ||
| if ambientPath != "" { | ||
| if ambientPath != bundledPath { |
There was a problem hiding this comment.
don't you need to check for bundledPath != "" here?
There was a problem hiding this comment.
Nope, if bundledPath is empty then this was a non-bundled plugin (like a provider) and we want to warn. "path" != "" is true and we log the warning.
abhinav
approved these changes
Jul 27, 2023
sdk/go/common/workspace/plugins.go
Outdated
Comment on lines
1823
to
1833
| return &PluginInfo{ | ||
| Kind: kind, | ||
| Name: name, | ||
| Path: filepath.Dir(ambientPath), | ||
| }, ambientPath, nil | ||
| } else if bundledPath != "" { | ||
| return &PluginInfo{ | ||
| Kind: kind, | ||
| Name: name, | ||
| Path: filepath.Dir(bundledPath), | ||
| }, bundledPath, nil |
There was a problem hiding this comment.
minor: I think this would be cleaner if we didn't have the two separate returns. We could have a block calculating a plugin path, and then just building PluginInfo from that.
pluginPath := bundledPath
if ambientPath != "" {
if ambientPath != pluginPath {
// warn
}
pluginPath = ambientPath
}
if pluginPath != "" {
return &PluginInfo{..}, pluginPath, nil
}| @@ -1080,15 +1146,74 @@ func TestBundledPluginSearch(t *testing.T) { | |||
| err = os.WriteFile(ambientPath, []byte{}, 0o700) //nolint: gosec | |||
| require.NoError(t, err) | |||
|
|
|||
| d := &mockSink{t: t} | |||
There was a problem hiding this comment.
I think for the cases where you don't care about the output, diagtest.LogSink may be preferable since it'll log to the testing.T:
There was a problem hiding this comment.
! I was looking for something like this
I'll rework things to use that module.
| t.Setenv("PATH", pathDir) | ||
| ambientPath := filepath.Join(pathDir, "pulumi-resource-mock") | ||
| err := os.WriteFile(ambientPath, []byte{}, 0o700) //nolint: gosec | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
This is fine as-is, but you may find it easier to not have a mock sink and instead use a diag.DefaultSink with a buffer for stderr.
var stderr bytes.Buffer
d := diag.DefaultSink(
iotest.LogWriter(t), // stdout
&stderr,
diag.FormatOptions{Color: "never"},
)
// ...
assert.Regexp(t, `using \S+/pulumi-resource-mock from \$PATH` stderr.String())This way you're also validating the final message and where it's printed instead of the printf string template.
By default Pulumi will load ambient plugins from $PATH before looking in the plugins directory or at bundled plugins. While this is very useful for development it often causes confusion when people have forgotten that they have plugins left on $PATH. This makes the use of these $PATH plugins a diagnostic warning to try and make that failure mode a little less silent. Normal users shouldn't ever have plugins on $PATH and so won't see this new warning.
Frassle
force-pushed
the
fraser/warnAmbient
branch
from
76ee3ac
to
a5b1590
Compare
|
bors merge |
bors bot
added a commit
that referenced
this pull request
Jul 27, 2023
13607: Warn about ambient plugins loaded from $PATH r=Frassle a=Frassle <!--- 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. --> By default Pulumi will load ambient plugins from $PATH before looking in the plugins directory or at bundled plugins. While this is very useful for development it often causes confusion when people have forgotten that they have plugins left on $PATH. This makes the use of these $PATH plugins a diagnostic warning to try and make that failure mode a little less silent. Normal users shouldn't ever have plugins on $PATH and so won't see this new warning. ## Checklist - [x] I have run `make tidy` to update any new dependencies - [x] I have run `make lint` to verify my code passes the lint check - [x] I have formatted my code using `gofumpt` <!--- Please provide details if the checkbox below is to be left unchecked. --> - [x] I have added tests that prove my fix is effective or that my feature works <!--- User-facing changes require a CHANGELOG entry. --> - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Cloud, 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 Cloud API version <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. --> Co-authored-by: Fraser Waters <fraser@pulumi.com>
|
Build failed: |
|
Some automation api tests hit the race detector, looks the same as what Kyle opened an issue on the other day. bors retry |
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
Frassle
added a commit
that referenced
this pull request
Aug 8, 2023
By default Pulumi will load ambient plugins from $PATH before looking in the plugins directory or at bundled plugins. While this is very useful for development it often causes confusion when people have forgotten that they have plugins left on $PATH. This makes the use of these $PATH plugins a diagnostic warning to try and make that failure mode a little less silent. Normal users shouldn't ever have plugins on $PATH and so won't see this new warning. Re-instates #13607 with a fix for symlinks included.
Frassle
added a commit
that referenced
this pull request
Aug 8, 2023
By default Pulumi will load ambient plugins from $PATH before looking in the plugins directory or at bundled plugins. While this is very useful for development it often causes confusion when people have forgotten that they have plugins left on $PATH. This makes the use of these $PATH plugins a diagnostic warning to try and make that failure mode a little less silent. Normal users shouldn't ever have plugins on $PATH and so won't see this new warning. Re-instates #13607 with a fix for symlinks included.
Frassle
added a commit
that referenced
this pull request
Aug 8, 2023
By default Pulumi will load ambient plugins from $PATH before looking in the plugins directory or at bundled plugins. While this is very useful for development it often causes confusion when people have forgotten that they have plugins left on $PATH. This makes the use of these $PATH plugins a diagnostic warning to try and make that failure mode a little less silent. Normal users shouldn't ever have plugins on $PATH and so won't see this new warning. Re-instates #13607 with a fix for symlinks included.
6 tasks
bors bot
added a commit
that referenced
this pull request
Aug 8, 2023
13670: Warn about ambient plugins loaded from $PATH r=Frassle a=Frassle <!--- 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. --> By default Pulumi will load ambient plugins from $PATH before looking in the plugins directory or at bundled plugins. While this is very useful for development it often causes confusion when people have forgotten that they have plugins left on $PATH. This makes the use of these $PATH plugins a diagnostic warning to try and make that failure mode a little less silent. Normal users shouldn't ever have plugins on $PATH and so won't see this new warning. Re-instates #13607 with a fix for symlinks included. ## Checklist - [x] I have run `make tidy` to update any new dependencies - [x] I have run `make lint` to verify my code passes the lint check - [x] I have formatted my code using `gofumpt` <!--- Please provide details if the checkbox below is to be left unchecked. --> - [x] I have added tests that prove my fix is effective or that my feature works <!--- User-facing changes require a CHANGELOG entry. --> - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Cloud, 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 Cloud API version <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. --> Co-authored-by: Fraser Waters <fraser@pulumi.com>
bors bot
added a commit
that referenced
this pull request
Aug 8, 2023
13670: Warn about ambient plugins loaded from $PATH r=Frassle a=Frassle <!--- 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. --> By default Pulumi will load ambient plugins from $PATH before looking in the plugins directory or at bundled plugins. While this is very useful for development it often causes confusion when people have forgotten that they have plugins left on $PATH. This makes the use of these $PATH plugins a diagnostic warning to try and make that failure mode a little less silent. Normal users shouldn't ever have plugins on $PATH and so won't see this new warning. Re-instates #13607 with a fix for symlinks included. ## Checklist - [x] I have run `make tidy` to update any new dependencies - [x] I have run `make lint` to verify my code passes the lint check - [x] I have formatted my code using `gofumpt` <!--- Please provide details if the checkbox below is to be left unchecked. --> - [x] I have added tests that prove my fix is effective or that my feature works <!--- User-facing changes require a CHANGELOG entry. --> - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Cloud, 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 Cloud API version <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. --> Co-authored-by: Fraser Waters <fraser@pulumi.com>
bors bot
added a commit
that referenced
this pull request
Aug 8, 2023
13670: Warn about ambient plugins loaded from $PATH r=Frassle a=Frassle <!--- 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. --> By default Pulumi will load ambient plugins from $PATH before looking in the plugins directory or at bundled plugins. While this is very useful for development it often causes confusion when people have forgotten that they have plugins left on $PATH. This makes the use of these $PATH plugins a diagnostic warning to try and make that failure mode a little less silent. Normal users shouldn't ever have plugins on $PATH and so won't see this new warning. Re-instates #13607 with a fix for symlinks included. ## Checklist - [x] I have run `make tidy` to update any new dependencies - [x] I have run `make lint` to verify my code passes the lint check - [x] I have formatted my code using `gofumpt` <!--- Please provide details if the checkbox below is to be left unchecked. --> - [x] I have added tests that prove my fix is effective or that my feature works <!--- User-facing changes require a CHANGELOG entry. --> - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Cloud, 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 Cloud API version <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. --> Co-authored-by: Fraser Waters <fraser@pulumi.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
By default Pulumi will load ambient plugins from $PATH before looking in the plugins directory or at bundled plugins.
While this is very useful for development it often causes confusion when people have forgotten that they have plugins left on $PATH.
This makes the use of these $PATH plugins a diagnostic warning to try and make that failure mode a little less silent.
Normal users shouldn't ever have plugins on $PATH and so won't see this new warning.
Checklist
make tidyto update any new dependenciesmake lintto verify my code passes the lint checkgofumptmake changelogand committed thechangelog/pending/<file>documenting my change