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

Add marketplace listings API endpoints #929

Merged
merged 5 commits into from
Sep 12, 2017
Merged

Add marketplace listings API endpoints #929

merged 5 commits into from
Sep 12, 2017

Conversation

greysteil
Copy link
Contributor

That. 🎁

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 99.265% when pulling 7768855 on greysteil:add-marketplace-api into 1f43830 on octokit:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 99.265% when pulling 7768855 on greysteil:add-marketplace-api into 1f43830 on octokit:master.

@coveralls
Copy link

coveralls commented Sep 7, 2017

Coverage Status

Coverage decreased (-0.01%) to 99.265% when pulling eddf552 on greysteil:add-marketplace-api into 1f43830 on octokit:master.

@greysteil
Copy link
Contributor Author

@tarebyte are you the right person to review?

@tarebyte tarebyte self-requested a review September 7, 2017 22:12
@coveralls
Copy link

coveralls commented Sep 10, 2017

Coverage Status

Coverage decreased (-0.01%) to 99.265% when pulling 6e766af on greysteil:add-marketplace-api into af1a252 on octokit:master.

@greysteil
Copy link
Contributor Author

@sbarnekow - FYI, I wrote this while I was getting a feel for the API. Should make integrating with marketplace even easier for anyone else written in Ruby.

Copy link
Member

@tarebyte tarebyte 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 the PR, a few small changes and this will be good to go!

# Methods for the Marketplace Listing API
#
# @see https://developer.github.com/v3/apps/marketplace/
module MarketplaceListings
Copy link
Member

Choose a reason for hiding this comment

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

Let's go ahead and rename this to Marketplace

def plan_for_account(account_id, options = {})
opts = ensure_api_media_type(:marketplace, options)
get "/marketplace_listing/accounts/#{account_id}", opts
rescue Octokit::NotFound
Copy link
Member

Choose a reason for hiding this comment

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

Since this isn't consistent with the rest of Octokit, let's go ahead and remove this rescue so the error bubbles up.

@@ -15,6 +15,7 @@ module Preview
:projects => 'application/vnd.github.inertia-preview+json'.freeze,
:traffic => 'application/vnd.github.spiderman-preview'.freeze,
:integrations => 'application/vnd.github.machine-man-preview+json'.freeze,
:marketplace => 'application/vnd.github.valkyrie-preview+json'.freeze,
Copy link
Member

Choose a reason for hiding this comment

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

👍


private

def new_jwt_token
Copy link
Member

Choose a reason for hiding this comment

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

Since this is now used in two different test files, let's move this into the spec/helper.rb so we don't repeat logic.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 99.279% when pulling 9fbb40f on greysteil:add-marketplace-api into af1a252 on octokit:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 99.279% when pulling 9fbb40f on greysteil:add-marketplace-api into af1a252 on octokit:master.

@greysteil
Copy link
Contributor Author

@tarebyte - updated and 💚

Copy link
Member

@tarebyte tarebyte left a comment

Choose a reason for hiding this comment

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

Sorry noticed this after I ✅ the PR.

end

# List all GitHub accounts on a specific plan
#
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind adding the plan parameter here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

end

# Get the plan associated with a given GitHub account
#
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind adding the account_id parameter here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 99.279% when pulling fe3c15d on greysteil:add-marketplace-api into af1a252 on octokit:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 99.279% when pulling fe3c15d on greysteil:add-marketplace-api into af1a252 on octokit:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 99.279% when pulling 19e44ec on greysteil:add-marketplace-api into af1a252 on octokit:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 99.279% when pulling 19e44ec on greysteil:add-marketplace-api into af1a252 on octokit:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 99.279% when pulling 50a04e2 on greysteil:add-marketplace-api into af1a252 on octokit:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 99.279% when pulling 50a04e2 on greysteil:add-marketplace-api into af1a252 on octokit:master.

@tarebyte tarebyte merged commit e914c26 into octokit:master Sep 12, 2017
@tarebyte
Copy link
Member

🍰

@greysteil greysteil deleted the add-marketplace-api branch September 25, 2017 19:11
@kytrinyx kytrinyx mentioned this pull request Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants