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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[scan] fix scan regression caused by not setting slack_default_payloads option #17923

Merged
merged 5 commits into from
Jan 15, 2021

Conversation

rogerluan
Copy link
Member

@rogerluan rogerluan commented Jan 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

Resolves #17915
Resolves #17898
This PR fixes a regression introduced in #17866

This PR supersedes #17919 (also please do read the comments there, for extra context!)

Description

By passing nil in scan's slack_default_payloads option (or omitting), then slack action (called down the road within scan) would crashes.
Slack action's default_payloads option is non-null and has a default value. Theoretically, if you pass nil to it, it should pick up the default value and just work.
However, in this very specific scenario, scan was calling the Slack action without invoking the options initializer - it was just passing a raw hash to Slack's runner. Naturally, by not invoking the options initializer, the default values wouldn't be loaded into Slack's options.

So the fix for this issue is just the first commit, where I modify how scan invokes Slack action.

The subsequent commits are meant to improve code by making it more consistent, by swapping Fastlane::ConfigurationHelper.parse with FastlaneCore::Configuration.create. The former was pretty much only used in this file, while the latter is being used in over 300+ occurrences - seems just like the right way to do it 馃槉

I also cleaned up the test I had written earlier.

NOTE: I'm not too happy with the changes I had to made here 3e80113 - not sure if what we had before is an anti-pattern, or if what I did is an anti-pattern, but neither looks/feels great 馃槵 I appreciate any and every feedback wrt these specs 馃檹

Testing Steps

Demo project, kindly provided by @crazymanish: Demo.zip

Before (as of master):

cd into the demo project, run bundle install, then bundle exec fastlane test. It will crash with undefined method map' for nil:NilClass` error.

Now (in this branch):

To test this branch, modify your demo project's Gemfile as:

gem "fastlane", :git => "https://github.com/fastlane/fastlane.git", :branch => "rogerluan-fix-scan-slack-integration"

And run bundle install to apply the changes.

Now if you run bundle exec fastlane test, it will still fail, but it's because we're providing a fake slack_url 馃槆
If you test this branch in your own project, it should succeed 馃憤

cc @crazymanish 馃

Reason was just consistency. The ConfigurationHelper was pretty much
only used in this file, while Configuration.create is being used
in over 300+ occurrences - seems the better way to do it.
Copy link
Member

@crazymanish crazymanish left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for the fix. Unit tests too 馃挭馃殌

@stherold
Copy link

stherold commented Jan 8, 2021

Looks good for us 馃憤

@ainame ainame self-requested a review January 8, 2021 11:58
Copy link
Contributor

@ainame ainame left a comment

Choose a reason for hiding this comment

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

Nice fix! I suggested how to test the change otherwise LGTM.

scan/lib/scan/slack_poster.rb Outdated Show resolved Hide resolved
@rogerluan rogerluan requested a review from ainame January 11, 2021 06:44
Copy link
Contributor

@ainame ainame left a comment

Choose a reason for hiding this comment

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

Well done馃憤

@mathaeus
Copy link

@rogerluan thanks for fixing this. Is there an estimated time of when this will be released? :)

@rockshassa
Copy link

Can we get this merged ASAP please? With the upcoming Feb deadline for enabling 2FA on apple developer accounts, we need this to be working.

@ainame
Copy link
Contributor

ainame commented Jan 15, 2021

@mathaeus @rockshassa Hi. Sorry for the inconvenience馃檱 @joshdholtz is responsible for final checks and release a new version in the core team's flow but he is busy due to a personal circumstances at this moment馃懚 So please bear with us for a while.

In the meantime, if you really need this change, you can still use this PR's branch in your project. Use below line in your Gemfile. I guess this is the best part of using an OSS project, right? 馃槢

gem "fastlane", :git => "https://github.com/fastlane/fastlane.git", :branch => "rogerluan-fix-scan-slack-integration"

@joshdholtz
Copy link
Member

Will get this merged and released tomorrow! Sorry about that

@mathaeus
Copy link

Thanks for the follow up @ainame and @joshdholtz. And congrats @joshdholtz 馃懚 !!

@rockshassa
Copy link

yup, thanks for the quick response @ainame and congrats @joshdholtz ! 馃檹

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.

I love this fix! Thank you so much for another awesome contribution 鉂わ笍

@joshdholtz joshdholtz merged commit ee9af63 into master Jan 15, 2021
@joshdholtz joshdholtz deleted the rogerluan-fix-scan-slack-integration branch January 15, 2021 16:27
@fastlane-bot
Copy link

Hey @rogerluan 馃憢

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 馃殌

Copy link

@fastlane-bot fastlane-bot left a comment

Choose a reason for hiding this comment

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

Congratulations! 馃帀 This was released as part of fastlane 2.172.0 馃殌

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