Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Property map keys are just strings #15767

Merged
merged 2 commits into from
Mar 25, 2024
Merged

Property map keys are just strings #15767

merged 2 commits into from
Mar 25, 2024

Conversation

Frassle
Copy link
Member

@Frassle Frassle commented Mar 23, 2024 •
edited

Description

property.Map is just a plain map in many cases, users will send dictionary like values using property maps, and there's no guarantee their keys will stick to any sort of constraints.

There are a number of places we a property map is used where we do know the keys should stick to some known constraints. Such as the property map for resource outputs, all the keys should be valid property names (which aren't just any string). But firstly that knowledge is given by context, not by the type property.Map and secondly we're not really doing any validation of those places yet to check things do abide by any constraints so just using string everywhere for now is probably more honest.

Checklist

  • I have run make tidy to update any new dependencies
  • I have run make lint to verify my code passes the lint check
    • I have formatted my code using gofumpt
  • I have added tests that prove my fix is effective or that my feature works
  • I have run make changelog and committed the changelog/pending/<file> documenting my change
  • Yes, there are changes in this PR that warrants bumping the Pulumi Cloud API version

@Frassle Frassle requested a review from iwahbe March 23, 2024 11:48
@pulumi-bot
Copy link
Contributor

pulumi-bot commented Mar 23, 2024 •
edited

Changelog

[uncommitted] (2024-03-25)

Features

  • [sdk/go] Make property.Map keyed by string not MapKey
    #15767

@Frassle
Property map keys are just strings
7715402 
Property.Map is just a plain map in many cases, users will send `dictionary`
like values using property maps, and there's no guarentee their keys will stick
to any sort of constraints.

There are a number of places we a property map is used where we do know the
keys should stick to some known constraints. Such as the property map for
resource outputs, all the keys _should_ be valid property names (which aren't
just any string). But firstly that knowledge is given by context, not by the
type `property.Map` and secondly we're not really doing any validation of those
places yet to check things do abide by any constraints so just using string
everywhere for now is probably more honest.
@Frassle Frassle marked this pull request as ready for review March 23, 2024 11:50
@Frassle Frassle requested a review from a team as a code owner March 23, 2024 11:50
iwahbe
iwahbe approved these changes Mar 25, 2024
View reviewed changes
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to remove a type when property.MapKey = string.

@Frassle @tgummerer
Update changelog
69cc79d 
Co-authored-by: Thomas Gummerer <t.gummerer@gmail.com>
@Frassle Frassle enabled auto-merge March 25, 2024 17:26
@Frassle Frassle added this pull request to the merge queue Mar 25, 2024
Merged via the queue into master with commit d7ec486 Mar 25, 2024
48 checks passed
@Frassle Frassle deleted the fraser/propertyMapKeys branch March 25, 2024 19:12
@justinvp justinvp mentioned this pull request Mar 27, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Mar 27, 2024
@justinvp
Prepare for v3.112.0 release (#15794)
13ef42b 
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)
@justinvp justinvp mentioned this pull request Mar 27, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Mar 28, 2024
@justinvp
Freeze v3.112.0 (#15799)
89cd536 
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)
@MJLongstreth-tiq MJLongstreth-tiq mentioned this pull request Apr 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgummerer tgummerer tgummerer approved these changes

@iwahbe iwahbe iwahbe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Frassle @pulumi-bot @tgummerer @iwahbe