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

Add option to throw on output toString and toJSON calls #14811

Merged
merged 9 commits into from Dec 20, 2023
Merged

Add option to throw on output toString and toJSON calls #14811

merged 9 commits into from Dec 20, 2023

Conversation

devnev
Copy link
Contributor

@devnev devnev commented Dec 10, 2023 •
edited

Description

This adds an environment variable that changes output's .toString and .toJSON methods to throw an exception instead of returning a string containing a warning.

This feature mitigates the issue described in #4261 by allowing users to opt in to having accidental toString calls (e.g. in a JS backtick string that is missing pulumi.interpolate) throw an exception. The behaviour ends up similar to the get() method of outputs, which throws an exception.

I don't know how to write tests for this change, or even test it manually.

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

@github-actions GitHub Actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation

@pulumi-bot
Copy link
Contributor

pulumi-bot commented Dec 10, 2023 •
edited

Changelog

[uncommitted] (2023-12-20)

Features

  • [sdk/nodejs] Add an environment variable (PULUMI_ERROR_OUTPUT_STRING) to throw when an output tries to convert to a string.

Frassle
Frassle reviewed Dec 12, 2023
View reviewed changes
sdk/nodejs/runtime/state.ts Outdated Show resolved Hide resolved
sdk/nodejs/runtime/settings.ts Outdated Show resolved Hide resolved
sdk/nodejs/output.ts Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation

@devnev
Copy link
Contributor Author

devnev commented Dec 13, 2023

Thanks for taking a look at the PR!

@github-actions GitHub Actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation

@github-actions GitHub Actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation

@github-actions GitHub Actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation

@devnev
Copy link
Contributor Author

devnev commented Dec 13, 2023

@Frassle should I be applying this to other SDKs, or is doing only the NodeJS SDK fine for now?

@Frassle Frassle self-assigned this Dec 15, 2023
@Frassle
Copy link
Member

Frassle commented Dec 15, 2023

should I be applying this to other SDKs, or is doing only the NodeJS SDK fine for now?

It would be nice if you did the others (well only python needs doing in this repo) but I'll accept this just changing node and I'll go fix the others up afterwards.

@devnev @Frassle
Include env name in changelog
cd1fff5 
Co-authored-by: Fraser Waters <frassle@gmail.com>
@github-actions GitHub Actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation

@github-actions GitHub Actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation

@devnev devnev requested a review from Frassle December 18, 2023 01:16
@Frassle
Copy link
Member

Frassle commented Dec 19, 2023 •
edited by pulumi-bot

/run-acceptance-tests
Please view the results of the acceptance tests Here

@devnev
Copy link
Contributor Author

devnev commented Dec 20, 2023

woops, forgot to run make format & lint on the python sdk after the additions. fixed now.

@Frassle
Copy link
Member

Frassle commented Dec 20, 2023 •
edited by pulumi-bot

/run-acceptance-tests
Please view the results of the acceptance tests Here

@github-actions GitHub Actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation

@Frassle Frassle added this pull request to the merge queue Dec 20, 2023
@Frassle
Copy link
Member

Frassle commented Dec 20, 2023

Thanks for this. I'll take a look at updating dotnet and java SDKs to use this envvar as well.

Merged via the queue into pulumi:master with commit 1b63aa7 Dec 20, 2023
8 of 9 checks passed
@devnev devnev deleted the throw-on-output-tostring-option branch December 21, 2023 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Frassle Frassle Frassle approved these changes

Assignees

@Frassle Frassle

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@devnev @pulumi-bot @Frassle