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

Support setting boolean properties with -c.extraMetadata #1674

Closed
jviotti opened this issue Jun 15, 2017 · 15 comments · Fixed by EHDFE/ehdev-shell#205, Thorium-Sim/thorium-kiosk#40 or Thorium-Sim/thorium-kiosk#41 · May be fixed by qcif/data-curator#563
Labels

Comments

@jviotti
Copy link
Member

jviotti commented Jun 15, 2017

  • Version: 18.6.2
  • Target: debian

I need to set a boolean value using --extraMetadata, however it seems that this option only supports strings, therefore something like:

--extraMetadata.updates.enabled=false

Results in:

{
    "updates": {
        "enabled": "false"
    }
}

I haven't tested it, but I suspect the same issue applies to numbers.

Of course I can workaround this limitation from my application code, however I wonder if we could add support for values other than strings.

Some thoughts:

  • We could check if the value equals false, true, or contains only numbers and coerce appropriately, however there might be cases where you really need to set something like "false"
  • Maybe there can be --extraMetadataBoolean and --extraMetadataNumber options?
@develar
Copy link
Member

develar commented Jun 16, 2017

19.4.0 allows you to specify extraMetadata in the config. Is it solution for you?

@develar
Copy link
Member

develar commented Jun 21, 2017

Fixed in 19.7.0

@jviotti
Copy link
Member Author

jviotti commented Jun 22, 2017

Sorry for the delay. I somehow missed the previous comment. The value of the variable I want to inject depends on certain make options, so having a configuration option would not be ideal.

Fixed in 19.7.0

What was the fix? Can you point me to a commit? I checked the CHANGELOG and the git history, but couldn't find anything relevant.

@develar
Copy link
Member

develar commented Jun 23, 2017

What was the fix?

new Ajv({allErrors: true, coerceTypes: true})

coerceTypes was set to true.

@jviotti
Copy link
Member Author

jviotti commented Jun 23, 2017

I just tested 19.7.0, but --extraMetadata.updates.enabled=false still results in a 'false' string.

@develar
Copy link
Member

develar commented Jun 23, 2017

-c.extraMetadata.updates.enabled=false

@develar
Copy link
Member

develar commented Jun 23, 2017

latest release is 19.8.0

--extraMetadata deprecation was not yet announced.

@jviotti
Copy link
Member Author

jviotti commented Jun 23, 2017

I upgraded to 19.8.0 and ran --config.extraMetadata.updates.enabled but still the same result.

@develar
Copy link
Member

develar commented Jun 23, 2017

Got it. extraMetadata typed as any and, so, not possible to convert. Well... is it ok for to specify it not in the CLI, but in the config file?

@develar develar reopened this Jun 24, 2017
@develar develar changed the title Support setting boolean properties with --extraMetadata Support setting boolean properties with -c.extraMetadata Jun 24, 2017
@develar
Copy link
Member

develar commented Jun 24, 2017

19.9.1

@jviotti
Copy link
Member Author

jviotti commented Jun 29, 2017

Well... is it ok for to specify it not in the CLI, but in the config file?

My particular use case is to disable auto updates on Debian and RPM packages (that we publish to Bintray repositories). Is there a better way to solve this with electron-publish?

@develar
Copy link
Member

develar commented Jun 30, 2017

  1. It is fixed. We just convert true/false strings to bool.
  2. extraMetadata will not work in this case because we build app for platform and then pack it into distributable formats. So, deb/rpm will use the same source dir.

Currently, I don't see way except somehow in the runtime detect. But — in any case auto update is not supported on Linux yet and will be supported only for AppImage. And AppImage sets some special ens, so, you can detect that app is running from AppImage.

@lurch
Copy link

lurch commented Jun 30, 2017

Minor correction to @jviotti 's previous comment - we're not actually auto-updating at the moment, but checking for a newer version of the app being available on the server than is running locally, and presenting a pop-up to the user if so. But obviously if the user has installed Etcher through some package-manager (like deb or rpm) we expect them to be updating through that, and we don't want to show them a pop-up that a newer version is available for manual download.

It is fixed. We just convert true/false strings to bool.

Thanks :)

@develar
Copy link
Member

develar commented Jun 30, 2017

@lurch So, use process.env.APPIMAGE != null to check that app running using AppImage.

@timo955
Copy link

timo955 commented Feb 6, 2021

9e77231

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment