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

[spaceship] migrate YAML.safe_load to support psych v4.0 #18825

Merged
merged 1 commit into from
Jun 10, 2021

Conversation

ainame
Copy link
Contributor

@ainame ainame commented Jun 8, 2021

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

Closes #18768
#17931 (comment)

psych v4.0.0 was released lately and it has a breaking change that breaks fastlane in two ways. One is to break http-cookie gem, another is by removing legacy positional parameters used in spaceauth_runner.rb.

JFYI: psych is a backend implementation of YAML used by default in Ruby. psych gem is categorised as "default" gem which means it's bundled in Ruby but you can update it to whatever version you want optionally. Ruby 3.0.1 bundles psych v3.3.0 but on the master branch of ruby/ruby it now points to v4.0.1, which means that it is very likely fastlane users using the next Ruby version (Ruby 3.1.0) are going to see errors due to psych v4.0.1.

Thanks to this ticket #18768, http-cookie is now fixed. This PR migrates spaceauth_runner.rb using the legacy positional parameters on YAML.safe_load which are removed in psych v4.0.0.

Description

It is a bit tricky that we need to maintain legacy positional parameters to keep Ruby 2.5 support. Otherwise, it is very straightforward.

v4.0.1 (bundled in Ruby head) https://github.com/ruby/psych/blob/v4.0.1/lib/psych.rb#L323
It's cleaned up. It no longer contains positional arguments for options.

  def self.safe_load yaml, permitted_classes: [], permitted_symbols: [], aliases: false, filename: nil, fallback: nil, symbolize_names: false, freeze: false<br class="Apple-interchange-newline">

v3.3.0 (bundled in Ruby 3.0.1) https://github.com/ruby/psych/blob/v3.3.0/lib/psych.rb#L329
There are positional parameters named as legacy to keep compatibility.

  def self.safe_load yaml, legacy_permitted_classes = NOT_GIVEN, legacy_permitted_symbols = NOT_GIVEN, legacy_aliases = NOT_GIVEN, legacy_filename = NOT_GIVEN, permitted_classes: [], permitted_symbols: [], aliases: false, filename: nil, fallback: nil, symbolize_names: false, freeze: false<br class="Apple-interchange-newline">

v3.0.2 (bundled in Ruby 2.5.0) https://github.com/ruby/psych/blob/v3.0.2/lib/psych.rb#L313
There is not keyword arguments for options yet.

  def self.safe_load yaml, whitelist_classes = [], whitelist_symbols = [], aliases = false, filename = nil, symbolize_names: false<br class="Apple-interchange-newline">

Testing Steps

I believe unit tests cover this change but you can give spaceauth a try to see if serialised cookie object is restored from yaml file.

@@ -72,6 +67,24 @@ def run
return self
end

def load_cookies(content)
# When Ruby 2.5 support is dropped, we can safely get rid of the latter branch.
if YAML.name == 'Psych' && Gem::Version.new(Psych::VERSION) >= Gem::Version.new('3.1')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can include it in the code as a comment as a reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm not really? There's no explanation for this logic there. I can add another comment on what this comparison means if you want?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've read the diff to understand the reasoning with Psych lib etc but I leave it to your criteria😁

@ainame ainame self-assigned this Jun 8, 2021
@ainame ainame marked this pull request as ready for review June 8, 2021 14:10
@ainame ainame changed the title [spaceship] Migrate YAML.safe_load to support Psych v4.0 [spaceship] Migrate YAML.safe_load to support psych v4.0 Jun 8, 2021
@ainame ainame changed the title [spaceship] Migrate YAML.safe_load to support psych v4.0 [spaceship] migrate YAML.safe_load to support psych v4.0 Jun 8, 2021
Copy link
Collaborator

@minuscorp minuscorp left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

👏 Thank you for finding and fixing this! You are the 3.0 hero ❤️

@joshdholtz joshdholtz merged commit d816062 into master Jun 10, 2021
@joshdholtz joshdholtz deleted the ai/migrate-yaml-loading branch June 10, 2021 20:47
@fastlane-bot
Copy link

Hey @ainame 👋

Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a week, and your PR will be included in the next one.

Please let us know if this change requires an immediate release by adding a comment here 👍
We'll notify you once we shipped a new release with your changes 🚀

@fastlane-bot
Copy link

Congratulations! 🎉 This was released as part of fastlane 2.185.1 🚀

@fastlane fastlane locked and limited conversation to collaborators Aug 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

psych v4.0 to break FASTLANE_SESSION functionality
4 participants