We want to add more test coverage to the display code. The best way to
do that is to add it to the engine tests, that already cover most of the
pulumi functionality.
It's probably not really possible to review all of the output, but at
least it gives us a baseline, which we can work with.
There's a couple of tests that are flaky for reasons I don't quite
understand yet. I marked them as to skip and we can look at them later.
I'd rather get in the baseline tests sooner, rather than spending a
bunch of time looking at that. The output differences also seem very
minor, so not super concerning.
The biggest remaining issue is that this doesn't interact well with the
Chdir we're doing in the engine. We could either pass the CWD through,
or just try to get rid of that Chdir. So this should only be merged
after https://github.com/pulumi/pulumi/pull/15607.
I've tried to split this into a few commits, separating out adding the
testdata, so it's hopefully a little easier to review, even though the
PR is still quite large.
One other thing to note is that we're comparing that the output has all
the same lines, and not that it is exactly the same. Because of how the
engine is implemented, there's a bunch of race conditions otherwise,
that would make us have to skip a bunch of tests, just because e.g.
resource A is sometimes deleted before resource B and sometimes it's the
other way around.
The biggest downside of that is that running with `PULUMI_ACCEPT` will
produce a diff even when there are no changes. Hopefully we won't have
to run that way too often though, so it might not be a huge issue?
---------
Co-authored-by: Fraser Waters <fraser@pulumi.com>
A resource `A` depends on a resource `B` if:
1. `B` is `A`'s `Provider`.
2. `B` is `A`'s `Parent`.
3. `B` appears in `A`'s `Dependencies`.
4. `B` appears in one or more of `A`'s `PropertyDependencies`.
5. `B` is referenced by `A`'s `DeletedWith` field.
While cases 1, 2, and 3 (providers, parents, and dependencies) are
handled fairly consistently, there have been a number of cases where the
newer features of `PropertyDependencies` (case 4) and `DeletedWith`
(case 5) have been neglected. This commit addresses some of these
omissions. Specifically:
* When refreshing state, it's important that we remove URNs that point
to resources that we've identified as deleted. Presently we check
pointers to parents and dependencies, but not property dependencies or
`deletedWith`. This commit fixes these gaps where dangling URNs could
lurk.
* Linked to the above, this commit extends snapshot integrity checks to
include property dependencies and `deletedWith`. Some tests that
previously used now invalid states have to be repaired or removed as a
result of this.
* Fixing snapshot integrity checking reveals that dependency graph
checks also fail to consider property dependencies and `deletedWith`.
This probably hasn't bitten us since property dependencies are currently
rolled up into dependencies (for temporary backwards compatibility) and
`deletedWith` is typically an optimisation (moreover, one that only
matters during deletes), so operations being parallelised due a
perceived lack of dependency still succeed. However, tests that
previously passed now fail as we can spot these races with our better
integrity checks. This commit thus fixes up dependency graph
construction and bulks out its test suite to cover the new cases.
These bugs were discovered as part of the investigation into #16052,
though they may not be directly responsible for it (though the issues
with dependency graphs are certainly a candidate).
<!---
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. -->
This adds a pseudo op `OpOutputChange` to the set of changes that are
recorded in `display.ResourceChanges` to count the number of output
changes, this is then included in the check used to evaluate the
`--expect-no-changes` flag
When resource outputs are registered, they are checked against their
previous value using existing functionality, the total count of changes
is then added
The internal capability is validated with an engine test, the cli is
validated using an integration test
This will break user workflows that depend on the previous behavior
Fixes#15564
## 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
- [ ] 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: Paul Roberts <proberts@pulumi.com>
<!---
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
This PR moves PluginKind to apitype to prevent circular dependencies
when adding apitype as a dependency of the workspace module.
It also re-exports PluginKind to keep backward compatibility
## 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. -->
This method already returns 4 different values, and we want to add more.
Refactor it so it returns a struct, to make adding additional return
values easier in the future.
The only real change here is a new test in
pkg/engine/lifecycletest/pulumi_test.go and the small fix up to the
`deploytest.ResourceMonitor` to return the information needed for that
test.
Noticed this while trying to prepare a PR for the transforms support.
These tests were checking that we mutated a field on the request object
itself. We no longer do that in the transform branch because everything
has to go through the transform callbacks as well but it caused these
tests to fail.
I've deleted these tests and written a new engine test to validate the
system output is actually as expected.
<!---
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. -->
Fixes https://github.com/pulumi/pulumi/issues/13968.
Fixes https://github.com/pulumi/pulumi/issues/8949.
This requires changing the parsing of URN's slightly, it is _very_
likely that
providers will need to update to handle URNs like this correctly.
This changes resource names to be `string` not `QName`. We never
validated this before and it turns out that users have put all manner of
text for resource names so we just updating the system to correctly
reflect that.
## 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. -->
<!---
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. -->
Fixes https://github.com/pulumi/pulumi/issues/14115.
This was missed as part of https://github.com/pulumi/pulumi/pull/13139.
Adds a new configure flag (sends_old_inputs_to_delete) which the engine
will now always set to true. If that's set providers can rely on the old
inputs being sent to delete, otherwise they'll get nil.
## 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
- [ ] 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. -->
<!---
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. -->
Likewise `require.NoError` instead of `require.Nil`, and `assert.Error`
rather than `assert.NotNil`.
The error variants of these functions print the errors nicer for test
failures using `Error()` rather than `GoString()`.
For bail errors this is _much_ better than the `result.Result` days
where we now get errors like:
```
Error: Received unexpected error:
BAIL: inner error
```
instead of:
```
Error: Expected nil, but got: &simpleResult{}
```
Also print the bail error in `TestPlan.Run` so we can see the
description of it.
<!---
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. -->
Fixes https://github.com/pulumi/pulumi/issues/13987.
This reworks the registry to better track provider instances such that
we can reuse unconfigured instances between Creates, Updates, and Sames.
When we allocate a provider instance in the registry for a Check call we
save it with the special id "unconfigured". This value should never make
its way back to program SDKs, it's purely an internal value for the
engine.
When we do a Create, Update or Same we look to see if there's an
unconfigured provider to use and if so configures that one, else it
starts up a fresh one. (N.B. Update we can assume there will always be
an unconfigured one from the Check call before).
This has also fixed registry Create to use the ID `UnknownID` rather
than `""`, have added some contract assertions to check that and fixed
up some test fallout because of that (the tests had been getting away
with leaving ID blank before).
## 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
- [ ] 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.
-->
- [ ] 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. -->
A big change for result.Result cleanup. This removes all references to
the Result type from pkg/engine. This was mostly just search replace.
Points to note, we still map to `result.Result` in pkg/backend (that
will be the next big result change to deal with), and a little bit of
fiddlyness with multiple error values in test_plan.go `runWithContext`.
<!---
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. -->
This PR fixes the inadvertent use of a closed plugin host in the
lifecycle tests. The tests override the host that is provided to the
engine, for good reasons, but that same host is re-used across multiple
engine operations. Since the engine closes the supplied host at the end
of each operation, subsequent operations are handed a closed host.
In order to detect engine bugs related to the use of a closed host (see
https://github.com/pulumi/pulumi/pull/14057), the fake host should
return an error if it is used after being closed (as does the real
host). This PR addresses this.
The detailed change is to shift to using a host factory that produces a
host in `TestOp.Run`. The `TestPlan` now takes a `TestUpdateOptions`
with `HostF` and an embedded `UpdateOptions`.
Note that two tests fail due to
https://github.com/pulumi/pulumi/pull/14057 which was being masked by
the problem that is fixed here. This PR disables those tests and the
other PR will re-enable them.
- `TestCanceledRefresh`
- `TestProviderCancellation`
## 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. -->
Similar to how https://github.com/pulumi/pulumi/pull/13953 moves some
code from sdk/go/common to /pkg. This display code is only used in /pkg,
another simple reduction of what's in sdk/go/common.
Fixes https://github.com/pulumi/pulumi/issues/5973
This adds two new properties to the RPC interfaces with providers.
Firstly we pass a new bool flag to Configure to tell the provider if it
will get sent old inputs as part of Diff.
Secondly we now pass the old inputs and the old outputs to Diff and
Update as opposed to just the old outputs.
The flag passed to Configure isn't strictly needed. Providers should be
able to tell that the old inputs aren't being sent to Diff and Updae by
virtue of them being `null/nil/None` rather than an empty map. For
DiffConfig this is the only way to detect this case because it's called
before Configure. But its still useful to send this explictly to
Configure to allow providers to return a configure error that they need
to be ran against a newer engine version.
There is one fairly major failure case that can happen with this feature
and providers who update to make use of this feature should ensure they
call this out in their changelog.
That is if a provider has been saving old inputs in outputs as a way to
get access to them in diff, and then updates to use these new protocol
properties instead and stops saving the inputs in outputs _and then_ the
user downgrades back to an old provider version things will probably get
_very_ confused.
This is implemented in the engine and interprets the empty string `""`
to inherit the value from the resource's parent if it exists signifying
that it was unspecified by the user's program. There is currently no way
to override this in a child to unset it when set by the parent, but can
be addressed by not parenting the resource to a resource with
`deletedWith` set.
This changes the provider registry to no longer load all the providers
from the old state on startup (in `NewRegistry`) instead the load logic
has been moved to the `Same` method. The step_executor and
step_generator have been fixed up to ensure that for cases where a
resource might not have had it's provider created yet (i.e. for DBR'ing
the old version of a resource, for refreshes or deletes) they ask the
`Deployment` to look up the provider in the old state and `Same` it in the
registry.
All of the above means we only load providers we're going to use (even
taking --targets into account).
One fix mot done in this change is to auto-update providers for deletes.
That is given a program state with two resources both using V1 of a
provider, if you run the program to update one of those resource to use
V2 of the provider but to delete the other resource currently we'll
still load V1 to do that delete. It _might_ be possible (although this
is definitly questionable) to see that another resource changed it's
provider from V1 to V2 and to just assume the same change should have
happened to the deleted resource.
This could be helpful for not loading old provider versions at all, but
can be done in two passes now pretty easily. Just run `up` without any
program changes except for the SDK version bump to update all the
provider references to V2 of the provider, then do another `up` that
deletes the second resource.
Fixes https://github.com/pulumi/pulumi/issues/12177.
Per team discussion, switching to gofumpt.
[gofumpt][1] is an alternative, stricter alternative to gofmt.
It addresses other stylistic concerns that gofmt doesn't yet cover.
[1]: https://github.com/mvdan/gofumpt
See the full list of [Added rules][2], but it includes:
- Dropping empty lines around function bodies
- Dropping unnecessary variable grouping when there's only one variable
- Ensuring an empty line between multi-line functions
- simplification (`-s` in gofmt) is always enabled
- Ensuring multi-line function signatures end with
`) {` on a separate line.
[2]: https://github.com/mvdan/gofumpt#Added-rules
gofumpt is stricter, but there's no lock-in.
All gofumpt output is valid gofmt output,
so if we decide we don't like it, it's easy to switch back
without any code changes.
gofumpt support is built into the tooling we use for development
so this won't change development workflows.
- golangci-lint includes a gofumpt check (enabled in this PR)
- gopls, the LSP for Go, includes a gofumpt option
(see [installation instrutions][3])
[3]: https://github.com/mvdan/gofumpt#installation
This change was generated by running:
```bash
gofumpt -w $(rg --files -g '*.go' | rg -v testdata | rg -v compilation_error)
```
The following files were manually tweaked afterwards:
- pkg/cmd/pulumi/stack_change_secrets_provider.go:
one of the lines overflowed and had comments in an inconvenient place
- pkg/cmd/pulumi/destroy.go:
`var x T = y` where `T` wasn't necessary
- pkg/cmd/pulumi/policy_new.go:
long line because of error message
- pkg/backend/snapshot_test.go:
long line trying to assign three variables in the same assignment
I have included mention of gofumpt in the CONTRIBUTING.md.
PluginSpec is used to specifiy a plugin, and is what is passed to things
like "Install". PluginInfo is used to refer to an installed plugin, and
so has extra data like file sizes, and time stamps, but does not include
things like plugin download url.
* Moving previewDigest and exporting it, closes#9851
* Moving previewDigest and exporting it, closes#9851
* Updating changelog-pending
* Go Mod Tidy
* replacing to local
* more go.mod changes
* reseting go mod
* full move
* Fixing golint
* No go.mod changes needed