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] parse response body as json if not already #18766

Merged
merged 6 commits into from Jun 23, 2021
Merged

[spaceship] parse response body as json if not already #18766

merged 6 commits into from Jun 23, 2021

Conversation

ilendemli
Copy link
Contributor

@ilendemli ilendemli commented May 28, 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

spaceship failed to parse an api-responses such as expired membership when running fastlane match. This MR checks if the respone body is an object of type String and parses the body as a JSON object if that is the case (better handling for #18431 and similar failures).

Description

updates handle_error and format_errors in Spaceshipt::ConnectAPIT::APIClient and checks wether response.body is empty or not. uses an empty object if String is empty. Then parses result as JSON if the result is tyoe of string

Testing Steps

rspec and rubocop went through as-well as bundle exec fastlane test

parse response body as json if not already
@google-cla google-cla bot added the cla: yes label May 28, 2021
@ilendemli ilendemli changed the title parse response body as json if not already [spaceship] parse response body as json if not already May 28, 2021
@max-ott
Copy link
Contributor

max-ott commented May 28, 2021

Hi @ilendemli 👋 ,
Looks like a great PR! Do you have any tests steps for us to test this via the spaceship playground or similar? What kind of "errors" / test cases did you use besides expired membership?

return body
end

return JSON.parse(body)
Copy link
Member

Choose a reason for hiding this comment

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

We might want to rescue an error here if it can't parse and display something somewhat user friendly 🤔 Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed this part of the code, but I am doing the same as in other places.

@ilendemli
Copy link
Contributor Author

ilendemli commented May 28, 2021

Hi @ilendemli 👋 ,
Looks like a great PR! Do you have any tests steps for us to test this via the spaceship playground or similar? What kind of "errors" / test cases did you use besides expired membership?

Hey @max-ott!

I traced the call:
Spaceship::ConnectAPI::Provisioning::Client sends a request by calling the request method from its superclass Spaceship::ConnectAPI::APIClient which is a subclass of Spaceship::Client. Spaceship::ConnectAPI::APIClient overrides the handle_error method but the passed object to Spaceship::Client is a String and not a JSON/Hash.

I am now doing the same as in fetch_olympus_session in Spaceship::Client#L542.

Not sure if I have missed something else.

EDIT: I don't have any steps i can give. I have projects with different teams where i changed the fastlane instance to my cloned repo and then debugged using a breakpoint and compared responses by stepping through fastlane.

Copy link

@dede760 dede760 left a comment

Choose a reason for hiding this comment

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

.

@fastlane fastlane deleted a comment from dede760 May 30, 2021
@fastlane fastlane deleted a comment from dede760 May 30, 2021
@ilendemli ilendemli requested a review from joshdholtz May 31, 2021 08:16
@google-cla
Copy link

google-cla bot commented Jun 11, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Jun 11, 2021
@joshdholtz
Copy link
Member

@googlebot I consent.

@google-cla google-cla bot added cla: yes and removed cla: no labels Jun 23, 2021
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.

Thanks for this PR! I added a little bit to it and some tests. I really appreciate the contribution ❤️

@joshdholtz joshdholtz merged commit 0a25f2f into fastlane:master Jun 23, 2021
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.186.0 🚀

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

5 participants