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

ElectronBuilder: Allow Date/Time deserialization #14205

Conversation

samford
Copy link
Member

@samford samford commented Dec 5, 2022

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

The ElectronBuilder strategy uses YAML#safe_load to parse YAML content and this limits deserialization to appropriate classes. We recently encountered a Tried to load unspecified class: Time error when using the ElectronBuilder strategy on a latest-mac.yml file containing releaseDate: 2022-12-01T02:02:46.419Z.

The electron-builder YAML files we usually encounter use single quotes around the releaseDate value to ensure it's treated as a string (e.g., releaseDate: '2022-10-12T17:55:26.718Z') and this is what we do in electron_builder_spec.rb. The aforementioned YAML file doesn't use single quotes around the value, so it's treated as a timestamp and apparently this makes Psych use Time (which #safe_load doesn't allow by default).

There's something to be said for upstream simply using single quotes around this value (the electron-builder documentation seems to suggest the value should be a string) but that's not something we can directly control and we may encounter other files like this in the future. With that in mind, this PR modifies the related #safe_load call to allow Time (and Date for good measure), which will resolve the aforementioned error and allow the ElectronBuilder strategy to work as expected in this scenario.

@BrewTestBot
Copy link
Member

Review period will end on 2022-12-06 at 02:43:06 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 5, 2022
Copy link
Member

@p-linnane p-linnane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Symbolic 👍. Thanks for turning this around so quickly!

The `ElectronBuilder` strategy uses `YAML#safe_load` to parse YAML
content and this limits deserialization to appropriate classes. We
recently encountered a `Tried to load unspecified class: Time` error
when using the `ElectronBuilder` strategy on a `latest-mac.yml` file
containing `releaseDate: 2022-12-01T02:02:46.419Z`.

The electron-builder YAML files we usually encounter use single
quotes around the `releaseDate` value to ensure it's treated as a
string (e.g., `releaseDate: '2022-10-12T17:55:26.718Z'`) and this is
what we do in `electron_builder_spec.rb`. The aforementioned YAML
file doesn't use single quotes around the value, so it's treated as
a timestamp and apparently this makes Psych use `Time` (which
`#safe_load` doesn't allow by default).

Seeing as we can't control the YAML content and there's a chance we
may encounter other files like this in the future, this commit
modifies the related `#safe_load` call to allow `Time` (and `Date`
for good measure). This will resolve the aforementioned error and
allow the `ElectronBuilder` strategy to work as expected in this
scenario.
@samford samford force-pushed the livecheck/electronbuilder-allow-date-time-deserialization branch from 2b8100d to e56735a Compare December 6, 2022 02:45
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 6, 2022
@BrewTestBot
Copy link
Member

Review period ended.

@samford samford merged commit 4a07924 into Homebrew:master Dec 6, 2022
@samford samford deleted the livecheck/electronbuilder-allow-date-time-deserialization branch December 6, 2022 04:24
@github-actions github-actions bot added the outdated PR was locked due to age label Jan 6, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants