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

No support for jsonc files #200

Closed
kilasuit opened this issue Oct 1, 2020 · 6 comments
Closed

No support for jsonc files #200

kilasuit opened this issue Oct 1, 2020 · 6 comments
Labels
enhancement New feature or request

Comments

@kilasuit
Copy link

kilasuit commented Oct 1, 2020

On a quick scan of this repo there's no support at all for using jsonc files, which whilst uncommon compared to normal json files should be allowed as ARM is happy with accepting deployments that use jsonc files.

@nerddtvg
Copy link

nerddtvg commented Oct 1, 2020

I would also like to see this!

@kilasuit
Copy link
Author

kilasuit commented Oct 1, 2020

Looking more and more into this there are a few parts to this.

-This codebase uses lines like this this & this that could be amended to match on either json/jsonc files

  • Jsonc files can be converted to objects in PowerShell v6= but not in v5.1 - as per discussion in this issue - Add .jsonc support to ConvertFrom-Json PowerShell/PowerShell#7436 - which for this tool I think should not be an issue anyway & if we made this a PowerShell v7.0 or above compatable tool this would also allow us to remove this file & the overriding ConvertFrom-Json funtion completely

So I propose 2 things

  • edit all the code that is looking for explicit .json to match on both .json & .jsonc

either

  • make this a PowerShell v7 and above tool & remove the need for the custom ConvertFrom-Json function.
  • rename ConvertFrom-Json to either ConvertFromJson or ConvertFrom-JsonWithComment and update the codebase accordingly as it's better not to override core powershell functionality for others (even if technically it provides better functionality)

My preference would be to update to be compliant with PowerShell 7 as that is what many build & release pipelines will be using in other tooling (Azure DevOps / GitHub Actions) so should no be an issue & is where most likely run the arm-ttk anyway - I know that is where we would be running it using Sam Cogans extension (which would also need a PR to incorporate this) however if you would prefer to keep this compliant with v5.1 I'd be happy to put in the PR for the change of the ConvertFrom-Json function as opposed to removing it.

@bmoore-msft
Copy link
Contributor

Are you asking for jsonc because you want support for comments in the files? Or because you want no quotes around properties (for example)?

If the former, I don't mind a PR that looks for *.json and *.jsonc - if the latter, ARM doesn't actually like that so............

@bmoore-msft bmoore-msft added the enhancement New feature or request label Oct 1, 2020
@kilasuit
Copy link
Author

kilasuit commented Oct 2, 2020

My use case is just for comments in series of files where they are complex (lots of linked templates being used)
I'd be happy to add a PR for this I don't mind a PR that looks for *.json and *.jsonc and also which ever of these two suggestions at the same time

  • make this a PowerShell v7 and above tool & remove the need for the custom ConvertFrom-Json function.
  • rename ConvertFrom-Json to either ConvertFromJson or ConvertFrom-JsonWithComment and update the codebase accordingly as it's better not to override core powershell functionality for others (even if technically it provides better functionality)

kilasuit added a commit to kilasuit/arm-ttk that referenced this issue Oct 2, 2020
kilasuit added a commit to kilasuit/arm-ttk that referenced this issue Oct 2, 2020
@bmoore-msft
Copy link
Contributor

I would separate things a bit...

  1. the need to handle comments, which are one "feature" of jsonc that ARM also supports, this should be as simple as looking for .json or .jsonc - maybe slightly more since mainTemplate/azuredeploy are specifically used but I think that should be relatively straightforward.

re: PSv7 - I would separate this work item and probably wait until V7 GAs, we've run into an issue or two in V7 and given ConvertFrom-JSON is working would wait until there is a greater need for that...

Thoughts?

@kilasuit
Copy link
Author

kilasuit commented Oct 2, 2020

Yeah I was going to raise as 2 PR's as opposed to one anyway (already have started for jsonc support in #206 ) & have moved the conversation about clean up into #207

@bmoore-msft bmoore-msft added this to To do in Co (M3) - Variable Eval via automation Mar 26, 2021
@bmoore-msft bmoore-msft removed this from To do in Co (M3) - Variable Eval Mar 29, 2021
StartAutomating added a commit to StartAutomating/arm-ttk that referenced this issue Jun 13, 2021
@bmoore-msft bmoore-msft moved this from To do to In progress in Co (M4) - Misc Bugs Jun 15, 2021
bmoore-msft pushed a commit that referenced this issue Jun 18, 2021
* Fixing #200 - Support for JSONC files

* Adding test for JSONC support.
@bmoore-msft bmoore-msft moved this from In progress to Done in Co (M4) - Misc Bugs Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Development

No branches or pull requests

3 participants