Asset and Archive can have missing contents #15736
Conversation
Changelog[uncommitted] (2024-03-20)Bug Fixes
|
Frassle
force-pushed
the
fraser/missingContents
branch
2 times, most recently
from
6b4d70d
to
8632428
Compare
Frassle
force-pushed
the
fraser/missingContents
branch
from
8632428
to
81f8243
Compare
| // If we have a hash we can check if that's the "zero hash" and if so then we know the assets is just empty. If the | ||
| // hash does not equal the empty hash then we know this is a _placeholder_ archive where the contents are just | ||
| // currently not known. If we don't have a hash then we can't tell the difference and assume it's just empty. | ||
| if a.Hash == "" || a.Hash == "5f70bf18a086007016e948b04aed3b82103a36bea41755b6cddfaf10ace3c6ef" { |
There was a problem hiding this comment.
Where does this value come from?
There was a problem hiding this comment.
It's the hash you get from hashing an AssetsArchive that's empty
|
This works for me I think. Coming from FP it's not the easiest to follow and control all the ways a type can be constructed (introduction forms). Just a quick note that there was this "fourth case" prior to the change in this PR, there's even a comment about it, there's something about an "we will consider this to be an empty assets archive then with zero assets". https://github.com/pulumi/pulumi/blob/master/sdk/go/common/resource/archive/archive.go#L295 Perhaps could be interesting to see some tests that exercise the empty archive through the public-exported methods. |
Yeh this is true, but I don't think we realised at the time. When doing that PR (#14007) we were trying to fix the asserts being hit where Assets, Path, and URI were all zero. We thought given the asserts where checking one of those three was true, the "right" thing was to make sure that however the Archive got created it would return true for one of them. Turns out that was the wrong call because of cases like #15729 where an Archive really does only have Hash set.
My kingdom for proper discriminate unions 😢 |
|
FWIW #15729 is kind of on the fence.. HasContents() may be taken to mean - does this thing has contents in a platonic sense, to which the right answer is yes, this archive used to have some contents which we lost and only have a hash retained. But it is currently taken by the bridge to mean "does this thing have contents I can retrieve right now" to which the answer is no (because we lost them). BW-compat is king so reverting to the prior behavior is very reasonable, but this kind of API is error prone and I wouldn't mind using more suggestive words in the bridge to describe what it's doing. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Tentative changelog: ### Features - [docs] Implement constructor syntax examples for every resource in typescript, python, csharp and go [#15624](#15624) - [engine] Send output values with property dependency information to transform functions [#15637](#15637) - [engine] Add a --continue-on-error flag to pulumi destroy [#15727](#15727) - [sdk/go] Make `property.Map` keyed by `string` not `MapKey` [#15767](#15767) - [sdk/python] Improve the error message when depends_on is passed objects of the wrong type [#15737](#15737) ### Bug Fixes - [auto/{go,nodejs,python}] Make sure to read complete lines before trying to deserialize them as engine events [#15778](#15778) - [cli/plugin] Fix installing local language plugins on Windows [#15715](#15715) - [engine] Don't delete stack outputs on failed deployments [#15754](#15754) - [engine] Fix a panic when updating provider version in a run using --target [#15716](#15716) - [engine] Handle that Assets & Archives can be returned from providers without content. [#15736](#15736) - [engine] Fix the engine trying to delete a protected resource caught in a replace chain [#15776](#15776) - [sdkgen/docs] Add missing newline for `Coming soon!` [#15783](#15783) - [programgen/dotnet] Fix generated code for a list of resources used in resource option DependsOn [#15773](#15773) - [programgen/{dotnet,go}] Fixes emitted code for object expressions assigned to properties of type Any [#15770](#15770) - [sdk/go] Fix lookup of plugin and program dependencies when using Go workspaces [#15743](#15743) - [sdk/nodejs] Export automation.tag.TagMap type [#15774](#15774) - [sdk/python] Wait only for pending outputs in the Python SDK, not all pending asyncio tasks [#15744](#15744) ### Miscellaneous - [sdk/nodejs] Reorganize function serialization tests [#15753](#15753) - [sdk/nodejs] Move mockpackage tests to closure integration tests [#15757](#15757)
Tentative changelog: ### Features - [docs] Implement constructor syntax examples for every resource in typescript, python, csharp and go [#15624](#15624) - [docs] Implement YAML constructor syntax examples in the docs [#15791](#15791) - [engine] Send output values with property dependency information to transform functions [#15637](#15637) - [engine] Add a --continue-on-error flag to pulumi destroy [#15727](#15727) - [sdk/go] Make `property.Map` keyed by `string` not `MapKey` [#15767](#15767) - [sdk/nodejs] Make function serialization work with typescript 4 and 5 [#15761](#15761) - [sdk/python] Improve the error message when depends_on is passed objects of the wrong type [#15737](#15737) ### Bug Fixes - [auto/{go,nodejs,python}] Make sure to read complete lines before trying to deserialize them as engine events [#15778](#15778) - [cli/plugin] Fix installing local language plugins on Windows [#15715](#15715) - [engine] Don't delete stack outputs on failed deployments [#15754](#15754) - [engine] Fix a panic when updating provider version in a run using --target [#15716](#15716) - [engine] Handle that Assets & Archives can be returned from providers without content. [#15736](#15736) - [engine] Fix the engine trying to delete a protected resource caught in a replace chain [#15776](#15776) - [sdkgen/docs] Add missing newline for `Coming soon!` [#15783](#15783) - [programgen/dotnet] Fix generated code for a list of resources used in resource option DependsOn [#15773](#15773) - [programgen/{dotnet,go}] Fixes emitted code for object expressions assigned to properties of type Any [#15770](#15770) - [sdk/go] Fix lookup of plugin and program dependencies when using Go workspaces [#15743](#15743) - [sdk/nodejs] Export automation.tag.TagMap type [#15774](#15774) - [sdk/python] Wait only for pending outputs in the Python SDK, not all pending asyncio tasks [#15744](#15744) ### Miscellaneous - [sdk/nodejs] Reorganize function serialization tests [#15753](#15753) - [sdk/nodejs] Move mockpackage tests to closure integration tests [#15757](#15757)
Description
Fixes #15729.
This fixes Assets and Archives to allow four states, as opposed to the three that #14007 had enforced.
An asset can either be Text, Path, Uri, or none. That is
IsText,IsPath, andIsURIcan all return false. Similarly for archives except with Assets instead of Text.This happens when a provider returns an Assert (or Archive) with a hash value set, but no other contents.
The Asset and Archive objects have been updated to handle this case, and a number of places in the CLI that asserted that one of IsText/IsPath/IsURI were true have been fixed up to handle the case of all three being false.
Checklist
make tidyto update any new dependenciesmake lintto verify my code passes the lint checkgofumptmake changelogand committed thechangelog/pending/<file>documenting my change