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

Treat config values that start with '0' as strings #4393

Merged
merged 2 commits into from
Apr 14, 2020
Merged

Conversation

justinvp
Copy link
Member

When setting structured config values using --path, we automatically treat values that can be converted into an integer (via strconv.Atoi) as an integer, rather than as a string.

However, this ends up converting values like "0123456" into the integer 123456, stripping the leading 0, which isn't desirable for values like commit SHAs, etc., where you want to keep the 0 (and keep it a string).

This change makes it so that values starting with 0 are not implicitly converted to an integer; instead such values will remain a string.

Fixes #4295

When setting structured config values using `--path`, we automatically
treat values that can be converted into an integer via `strconv.Atoi` as
an integer, rather than as a string.

However, this ends up converting values like "0123456" into the integer
123456, stripping the leading 0, which isn't desirable for values like
commit SHAs, etc., where you want to keep the 0 (and keep it a string).

This change makes it so that values starting with 0 are not implicitly
converted to an integer; instead such values will remain a string.
@justinvp justinvp requested a review from pgavlin April 14, 2020 18:27
@justinvp justinvp merged commit 3311965 into master Apr 14, 2020
@pulumi-bot pulumi-bot deleted the justin/config-zero branch April 14, 2020 19:40
@nesl247
Copy link
Contributor

nesl247 commented Apr 14, 2020

Out of curiosity, does this fix it for all SDKs? I see only changes to the go SDK. If it does, I'm a bit confused to how pulumi's code is structured.

@justinvp
Copy link
Member Author

justinvp commented Apr 14, 2020

Out of curiosity, does this fix it for all SDKs?

Yes. This code isn't specific to any SDK. We recently reorganized the repo to support go modules. The implementation lives under sdk/go/common/*, which is used by the CLI commands.

@nesl247
Copy link
Contributor

nesl247 commented Apr 15, 2020

@justinvp Should I open another issue for the confusion, because it doesn't really make sense when you read the README in sdk. Nor does it make sense that nodejs and such is there, and yet it's used by all the languages.

@justinvp
Copy link
Member Author

@nesl247 Suggestions and improvements are welcome! What do you think would help reduce the confusion here?

@nesl247
Copy link
Contributor

nesl247 commented Apr 15, 2020

I think that everything in sdk/* should be only the language SDKs as the README says. Anything pulumi uses internally should belong outside of that directory in pkg or whever.

@fa-bwilliams
Copy link

There are multiple cases where this creates issues. For example, setting a firewall destinationPort like '1433'. It should be a string but comes in as a number like 1433. In this case, using '01433' and substring(1) to remove the 0. Another example would be an IP address like '10.0.0.1'. Comes in as as number instead of a string. In this case, '010.0.0.1' does not make it a string but rather still a number. Using "r10.0.0.1" makes it a string and still works with substring(1).

We really need some structured control over this. Terraform uses 'type=' in their variable blocks to provide this control. We need a flag like --type with values like string, bool and number so that we can control what we need our values to be.

@Frassle
Copy link
Member

Frassle commented Jan 18, 2023

Project level config does give the tools to solve this now, it's just not quite plumbed into the config set commands yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't remove leading 0 when running config set
5 participants