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

Generate deployment client from the OpenAPI spec #1149

Merged
merged 23 commits into from
Oct 29, 2019

Conversation

indigok
Copy link

@indigok indigok commented Sep 16, 2019

Part of #1157. An updated spike of #1010 as the underlying api spec has changed. So much thanks to @kytrinyx for her work and @tarebyte for helping me get started ✨

This includes a breaking change for deployment_statuses and create_deployment_status. We're updating the parameter from deployment_url to use both repo and deployment_id as this is more consistent with other methods.

This also includes a new method, deployment_status, that was previously unsupported.

TODO:

  • Update testing for the breaking change in the client
  • Update the name from Spike

lib/spike.rb Outdated Show resolved Hide resolved
lib/spike.rb Outdated Show resolved Hide resolved
@indigok indigok marked this pull request as ready for review September 24, 2019 19:16
@indigok indigok changed the title Deployment spike Generate deployment client from the OpenAPI spec Oct 1, 2019
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.

This looks ✨ I don't want to merge it yet because we need to figure out how to keep a 4.x branch and a new 5.x branch instead of just merging master while we develop this.

Thanks for all of your hard work @hmharvey

desc "Generate the API client files based on the OpenAPI route docs."
task :generate do
require_relative "lib/openapi_client_generator"
OpenAPIClientGenerator::API.at(OasParser::Definition.resolve("../routes/openapi/api.github.com/index.json")) do |api|
Copy link
Member

Choose a reason for hiding this comment

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

This depends on the right path, but since we're still developing this I think it's alright.

# @param status_id [Integer] The ID of the status
# @return <Sawyer::Resource> A single deployment status
# @see https://developer.github.com/v3/repos/deployments/#get-a-single-deployment-status
def deployment_status(repo, deployment_id, status_id, options = {})
Copy link
Member

Choose a reason for hiding this comment

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

Just making a note that this is a breaking change, but we're ok with that. It matches the rest of the client.

lib/openapi_client_generator.rb Show resolved Hide resolved
def required_params
params = definition.parameters.select(&:required).reject {|param| ["owner", "accept"].include?(param.name)}
if definition.request_body
params += definition.request_body.properties_for_format("application/json").select { |param| definition.request_body.content["application/json"]["schema"]["required"].include? param.name }
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty long, can we make this a multiline block?

end

def self.resource_for_path(path)
return :unsupported unless path.include? "deployment"
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this into an Array? So that we can just add more resources easier?

def self.resource_for_path(path)
return :unsupported unless path.include? "deployment"
path_segments = path.split("/").reject{ |segment| segment == "" }
resource = path_segments[3]
Copy link
Member

Choose a reason for hiding this comment

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

This seems pretty magic, does this ever change?

Copy link
Author

Choose a reason for hiding this comment

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

It definitely will, but it's pretty consistent for the paths under "repos" which was my next focus. I can look further into it, but I felt like it was good enough for now 🤔

resource ||= "repositories"

supported_resources = ["deployments"]
return (supported_resources.include? resource) ? resource : :unsupported
Copy link
Author

Choose a reason for hiding this comment

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

@tarebyte how does this look? I don't believe return :unsupported unless path.include?(["deployment"]) is valid 🤔

@indigok
Copy link
Author

indigok commented Oct 4, 2019

This looks ✨ I don't want to merge it yet because we need to figure out how to keep a 4.x branch and a new 5.x branch instead of just merging master while we develop this.

Definitely 👍

@indigok indigok mentioned this pull request Oct 8, 2019
2 tasks
@indigok indigok merged commit 8b28668 into octokit:master Oct 29, 2019
@indigok indigok deleted the deployment-spike branch October 30, 2019 23:49
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.

2 participants