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.

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

[Go] Regression in multi-line string #12358

Closed
iwahbe opened this issue Mar 6, 2023 · 1 comment · Fixed by #13249
Closed

[Go] Regression in multi-line string #12358

iwahbe opened this issue Mar 6, 2023 · 1 comment · Fixed by #13249
Assignees
Labels
area/codegen SDK-gen, program-gen, convert impact/regression Something that used to work, but is now broken kind/bug Some behavior is incorrect or out of spec language/go resolution/fixed This issue was fixed

Comments

@iwahbe
Copy link
Member

iwahbe commented Mar 6, 2023

What happened?

Observing the diff https://github.com/pulumi/pulumi-akamai/actions/runs/4328374490/jobs/7558039509, we previously generated a JSON string literal as

pulumi.String(fmt.Sprintf(`  [
	  {
	    "name": "rule1",
	    "type": "erMatchRule",
	    "useRelativeUrl": "none",
	    "statusCode": 301,
	    "redirectURL": "https://www.example.com",
	    "matchURL": "example.com",
	    "useIncomingQueryString": false,
	    "useIncomingSchemeAndHost": false
	  },
	  {
	    "name": "rule2",
	    "type": "erMatchRule",
	    "matches": [
	      {
	        "matchType": "path",
	        "matchValue": "/example/website.html",
	        "matchOperator": "equals",
	        "caseSensitive": false,
	        "negate": false
	      }
	    ],
	    "useRelativeUrl": "copy_scheme_hostname",
	    "statusCode": 301,
	    "redirectURL": "/website.html",
	    "useIncomingQueryString": false,
	    "useIncomingSchemeAndHost": true
	  }

 ]
 `))

This isn't 100% optimal, since the call to fmt.Sprintf is not necessary, but its very human readable.

After upgrading from tf-bridge v3.40.0 to v3.41.0, the generated string looks like:

pulumi.String("  [\n  {\n    \"name\": \"rule1\",\n    \"type\": \"erMatchRule\",\n    \"useRelativeUrl\": \"none\",\n    \"statusCode\": 301,\n    \"redirectURL\": \"[https://www.example.com\](https://www.example.com/)",\n    \"matchURL\": \"example.com\",\n    \"useIncomingQueryString\": false,\n    \"useIncomingSchemeAndHost\": false\n  },\n  {\n    \"name\": \"rule2\",\n    \"type\": \"erMatchRule\",\n    \"matches\": [\n      {\n        \"matchType\": \"path\",\n        \"matchValue\": \"/example/website.html\",\n        \"matchOperator\": \"equals\",\n        \"caseSensitive\": false,\n        \"negate\": false\n      }\n    ],\n    \"useRelativeUrl\": \"copy_scheme_hostname\",\n    \"statusCode\": 301,\n    \"redirectURL\": \"/website.html\",\n    \"useIncomingQueryString\": false,\n    \"useIncomingSchemeAndHost\": true\n  }\n]\n")`

We loose the unnecessary fmt.Sprintf (good), and the multi-line string literal (bad).

Expected Behavior

Long multi-line strings are rendered as multi-line string literals (using `), not single line string literals (using ").

Steps to reproduce

Upgrade the codegen version used.

Output of pulumi about

NA

Additional context

The upgrade bumps the used version of pulumi/pkg from v3.55.0 to v3.56.0. The original issue is pulumi/pulumi-akamai#152.

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@iwahbe iwahbe added kind/bug Some behavior is incorrect or out of spec language/go area/codegen SDK-gen, program-gen, convert needs-triage Needs attention from the triage team impact/regression Something that used to work, but is now broken labels Mar 6, 2023
@justinvp justinvp self-assigned this Mar 7, 2023
@justinvp justinvp removed the needs-triage Needs attention from the triage team label Mar 7, 2023
@justinvp
Copy link
Member

justinvp commented Mar 7, 2023

Taking a look. I haven't yet found where this changed.

@justinvp justinvp removed their assignment Jun 20, 2023
@abhinav abhinav self-assigned this Jun 22, 2023
bors bot added a commit that referenced this issue Jun 22, 2023
13078: cli(state upgrade): Prompt for project names for detached stacks r=abhinav a=abhinav

When running 'pulumi state upgrade', supply the
ProjectsForDetachedStacks option to the file state backend so that we
get asked to fill in project names for stacks where we could not guess
them automatically.

The implementation of the prompt is straightforward:
For each stack, ask a question with the survey package
and feed the result back to the filestate backend.

Testing this is a bit complicated because terminals are involved.
The test for this uses the go-expect and vt10x libraries
recommended in the documentation for survey.
It uses them to simulate a terminal emulator and acts on the output.
The pty library is used to create a compatible pseduo-terminal.
Unfortunately, these test libraries rely on Unix APIs and are not
available on Windows, so the test will not run on Windows machines.

Resolves #12600

---

Preview

![Kapture 2023-05-31 at 19 25 57](https://github.com/pulumi/pulumi/assets/41730/69fcf37d-0267-40cc-9002-6514f1cf9ad5)


13249: fix(codegen/go): Use raw string literals for multiline-strings r=abhinav a=abhinav

We already have logic in place to generate mutliline strings with
backticks if they meet certain conditions in genTemplateExpression.
However, the logic came into effect only if the string was complex
enough to warrant use of `Sprintf`
because the function short circuits to just printing a string literal
if the string doesn't need a `Sprintf`.

This changes genStringLiteral to be responsible for the decision
of whether the string warrants backticks or not,
retaining the prior conditions on whether the string would benefit
from being split across multiple lines.

Resolves #12358


13252: Freeze v3.73.0 r=Zaid-Ajaj a=Zaid-Ajaj



Co-authored-by: Abhinav Gupta <abhinav@pulumi.com>
Co-authored-by: Zaid Ajaj <zaid.naom@gmail.com>
bors bot added a commit that referenced this issue Jun 22, 2023
13249: fix(codegen/go): Use raw string literals for multiline-strings r=abhinav a=abhinav

We already have logic in place to generate mutliline strings with
backticks if they meet certain conditions in genTemplateExpression.
However, the logic came into effect only if the string was complex
enough to warrant use of `Sprintf`
because the function short circuits to just printing a string literal
if the string doesn't need a `Sprintf`.

This changes genStringLiteral to be responsible for the decision
of whether the string warrants backticks or not,
retaining the prior conditions on whether the string would benefit
from being split across multiple lines.

Resolves #12358


13252: Freeze v3.73.0 r=Zaid-Ajaj a=Zaid-Ajaj



Co-authored-by: Abhinav Gupta <abhinav@pulumi.com>
Co-authored-by: Zaid Ajaj <zaid.naom@gmail.com>
@bors bors bot closed this as completed in da184df Jun 22, 2023
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/codegen SDK-gen, program-gen, convert impact/regression Something that used to work, but is now broken kind/bug Some behavior is incorrect or out of spec language/go resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants