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

[fastlane] Extend socket failure payload #16632

Merged
merged 8 commits into from Jul 6, 2020

Conversation

rhdeck
Copy link
Contributor

@rhdeck rhdeck commented Jun 19, 2020

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. (Socket server is mostly undocumented)

Motivation and Context

Fixing issue #16631.

When accessing fastlane over the socket server, only a little error information comes back, and user-based error messages (e.g. like the intended UX of FastlaneError) do not get remitted at all. The failureinformation array does not include the message, and is frankly just a mess of backtraces that have limited utility unless one is diving back through the ruby code.

Solution:

Extend the failure payload include the class as failure_class and message asfailure_message of the raised error in the payload to complement the backtrace in failure_information. The socket client can then decide how to better inform the user of what happened.

Description

Edited socket.server.rb to add the returns of message and class methods on the error object to the payload. Since both are part of StandardError this should work every time.

I ran this with a customized Gemfile to confirm it generates the results that help the user.

Overall this is a simple (four lines of new code, one line with an added comma) change that makes the socket server much more useful!

@rhdeck rhdeck changed the title Extend socket error Extend socket failure payload Jun 19, 2020
@rhdeck rhdeck marked this pull request as ready for review June 19, 2020 13:34
@minuscorp
Copy link
Collaborator

This should be in sync with the Swift client in order to read about that issues and raises them to the user somehow.

@rhdeck
Copy link
Contributor Author

rhdeck commented Jun 23, 2020

Id say go for it - make a PR to extend the swift client to be smarter about Fastlane error types and messages. For the purposes of this PR:

  1. swift isn’t my use case, so client side extension was not in my scope
  2. this PR enables such an extension by exposing the data on the socket server, and
  3. because it extends the failure payload shape rather than changing a key, the PR doesn’t prevent the swift client from either using what it had before or using this new info if a client is so inclined.

So no downside to the PR and a potential layup for someone who wants to update the swift extension with intelligence for responding to FL error types and messages.

@minuscorp
Copy link
Collaborator

minuscorp commented Jun 27, 2020

I don't see the point of integrating this on the Ruby side and expect to someone else to fix the Swift part 🤔 I tend to think that this PR is incomplete. So this indeed doesn't fix #16631 IMHO.

@rhdeck
Copy link
Contributor Author

rhdeck commented Jun 27, 2020

My use case is a OSS TS/JS-based client for Fastlane I am writing: https://github.com/rhdeck/fastlane-js. I am using the socket server to enable the cross-language support, much as FL-swift does. I submitted a PR for setting the port which was merged without incident. I appreciate the client isn't part of FL core, but it's really useful - especially for React Native.

I made sure that the PR would address the problem without causing more issues in swift-land. I am not a user of the swift client and don't feel qualified to submit a PR refactoring its error handling. In comparison, the server/ruby change is quite simple.

Maybe the issue is that I see the socket server and the swift client as separate concerns, where we can improve one and get value from it without having to improve the other? It's my hope that we can be expansive about functionality and do what's easy to add value to more people.

FWIW it was not I who added the swift label to this or to issue #16631, which related to the ruby socket server not the swift client.

Anyhow, it would help my consumers' experience a lot to get the messages, especially for FastlaneError situations where the message is key for the user to make a smart decision.

Let me know if me doing something additional would help. The goal is to contribute.

@rhdeck
Copy link
Contributor Author

rhdeck commented Jun 27, 2020

I took a look at the code in the swift client, and the crux of error handling is in SocketResponse and SocketClient, which basically dumps failure data to a log and shuts down the socket. (This use case explains the lack of data from the server - its not used for any flow control except hard stop.)

That said, we could surface the information to that log with the following inserted, starting on line 41 in SocketResponse.swift:

            } else if status == "failure" {
                guard var failureInformation = statusDictionary["failure_information"] as? [String]  else {
                    self = .parseFailure(failureInformation: ["Ruby server indicated failure but Swift couldn't receive it"])
                    return
                }
                if let failureClass = statusDictionary["failure_class"] failureInformation.append(failureClass)
                if let failureMessage = statusDictionary["failure_message"] failureInformation.append(failureMessage)
                self = .failure(failureInformation: failureInformation)
                return
            }

I'm glad to commit this change so that the client can profit from the additional data, but I'd appreciate the read of whether this would be a helpful addition, since it's not my use case.

How can I be most helpful?

@janpio
Copy link
Member

janpio commented Jun 28, 2020

Hey @rhdeck, sorry I misunderstood what you were doing there - for me the socket server was/is directly connected to Swift - hence the label.

But your project looks super interesting! Any docs on how (you will be able) to use it in JS?

@janpio janpio changed the title Extend socket failure payload [fastlane] Extend socket failure payload Jun 28, 2020
Copy link
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

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

As this PR only surfaces additional data, I think this is safe to merge as is.

If someone wants, please feel free to open a PR against the Swift client of this to utilize this new power and use the error class and message to improve any error messages in case of error.

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.

LGTM 🚀

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.

This looks good to me! And https://github.com/rhdeck/fastlane-js looks dope 😊 Excited to try it out!

@joshdholtz joshdholtz merged commit a9e1f73 into fastlane:master Jul 6, 2020
@rhdeck
Copy link
Contributor Author

rhdeck commented Jul 6, 2020

Working on it! This error payload will help a lot.

Your point about docs was well-taken...

@fastlane-bot
Copy link

Hey @rhdeck 👋

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.151.0 🚀

minuscorp pushed a commit to minuscorp/fastlane that referenced this pull request Jul 18, 2020
* Adding port setter to action interface of socket_server

* Trailing whitespace caused lint failure

* Adding error detail for socket server

* Fixing trailing whitespace

* Clearing blank spaces from blank line (thank you rubocop)
@fastlane fastlane locked and limited conversation to collaborators Sep 6, 2020
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.

None yet

6 participants