Enterprise specific api support

[joeyw]

• Management console
• Admin stats
• Search indexing

Hello. I came across this issue via Code Triage and I'm new to Octokit but looking at the source, is this the kind of things you are looking at as for the admin stats?

module Octokit
  class Client
    module AdminStats
      def all(hostname)
        get_stats(hostname, "all")
      end

      def repos(hostname)
        get_stats(hostname, "repos")
      end

      def hooks(hostname)
        get_stats(hostname, "hooks")
      end

      # continued with pages, orgs, etc.

      private

      def get_stats(hostname, metric)
        get("http://#{hostname}/api/v3/enterprise/#{metric}")
      end
    end
  end
end

If not then no problem but I have a question. How would I test such code if I don't myself have GitHub Enterprise and therefore no hostname? If there some kind of demo hostname that I could access to test the requests?

[joeyw]

The tests currently in the master branch rely on mocked requests/responses. Whenever I add new functionality I use a combination of what is in the documentation and output from curl, but since you and I don't have an enterprise endpoint to test against the latter is somewhat difficult and also why I haven't added this myself.

However if you still want to add the functionality based on documentation it is possible and then we could ping a GitHubber to test it fully before merging. You don't need to worry about passing the hostname around, the requests take a relative url. The hostname is setup in Octokit::Client and defaults to the GitHub api endpoint https://api.github.com. Enterprise users override this by creating a new instance of the client but everything else stays the same. The get request would end up being get "enterprise/#{metric}". Take a look at some of the other tests to see how they are mocked and create mocks based on the enterprise examples.

@joeyw Thanks a lot for that explanation. I'll look into that.

Also, one last small thing. I can't seem to get Octokit::Client::Stats to work. @client.contributors_stats('pengwynn/octokit') gives me a NoMethodError: undefined method 'contributors_stats' for #<Octokit::Client:0x00000002d04588>. What am I doing wrong?

[joeyw]

I'm not sure why it is throwing a NoMethodError, it should be raising a NotFound error since pengynn/octokit recently moved to the Octokit organization so the call would look like @client.contributors_stats('octokit/octokit.rb').

Can you provide a little more code that shows how you are creating @client?

Same code that's in the spec: @client = Octokit::Client.new(:login => 'sferik'). Even when I do @client.methods.include? :contributors_stats it returns false.

[joeyw]

Have you added a new spec or having issues in an existing one?

No, no. I've just ran the specs, the all passed, and then used the code from this spec to use the Octokit::Client::Stats.

@joeyw OK, so this is what I've written. I've also attached some similar documentation to stats.rb. You can have a look at admin_stats.rb here. Again, I'm rather new to this source code and so I'm not too sure about writing tests. If I'd write something similar to this, would that be fine? Also, if this is Enterprise specific, should it be in the Octokit::Client class?

[joeyw]

So we have a new AdminStats module we want to add. We start by adding a spec for it because tests are cool. Add a new spec file for AdminStats in admin_stats_spec.rb and this will go inside the spec/octokit/client folder.

spec/octokit/client/admin_stats_spec.rb

# -*- encoding: utf-8 -*-
require 'helper'

describe Octokit::Client::AdminStats do
end

Now we run bundle exec rake spec and it fails with the nice error uninitialized constant Octokit::Client::AdminStats (NameError). This is good because it means were on the right track.

Now we need to add the AdminStats module. To do that we add a new file admin_stats.rb inside the lib/octokit/client folder with a basic skelton for the module.

lib/octokit/client/admin_stats.rb

module Octokit
  class Client
    module AdminStats
    end
  end
end

Running the tests at this point will still give the same error as before. We need to wire up the actual module inside the octokit client.

Open lib/octokit/client.rb. You will see a lot of requires for the other modules, here is where we will require the new AdminStats module. Pick a spot, any spot, we just need to add require 'octokit/client/admin_stats'.

Next we need to include the new module so the methods are accessible, again you will see a big list of includes, towards the bottom or wherever makes sense add an include Octokit::Client::AdminStats.

Now running the tests again will pass, even though we haven't added anything useful yet. From here forward any new methods added to the AdminStats module will be accessible through Octokit.method_name. And that is how new modules are added.

Now we can finally start adding new methods. I usually play around with the api with curl before putting the test together to make sure I understand how everything is working but in this case were going to be working off spec only.

Let's add a new method getting all the stats since it is the first on the list. The endpoint is listed as /api/v3/enterprise/stats/ however we can exclude /api/v3/ because that is configured by the client and allows us to use relative urls. Our endpoint is enterprise/stats.

Lettuce add a test to admin_stats_spec.rb to add a new method named enterprise_stats. I think the name fits nicely with what we will be doing - getting all the enterprise stats. It seems to work nicely when limiting the stats to specifics such as enterprise_repos_stats as will be added later. (Include the s for tongue twisting, maybe alias the methods without it if it sounds better).

Octokit testing is pretty simple. Stub the request and response, call the method, check the method works assuming the server responds with the expected json.

This is what the test for .enterprise_stats might look like with some notes added.

spec/octokit/client/admin_stats_spec.rb

# -*- encoding: utf-8 -*-
require 'helper'

describe Octokit::Client::AdminStats do

  # Setup a client like we do in all the other tests, sferik for consistency.
  before do
    @client = Octokit::Client.new(:login => 'sferik')
  end

  # The method we will be testing.
  describe ".enterprise_stats" do

    # Description of what it does.
    it "returns all available stats" do

      # The url we will be stubbing so we dont hit the server for the test. We
      # We also stub the response to return the json from the documentation
      # example since we can't use curl to get our own in this case. We add it
      # to the spec/fixtures/enterprise_stats.json file.
      stub_get("/enterprise/stats/all").
        to_return(json_response('enterprise_stats.json'))

      # Call our new method
      enterprise_stats = @client.enterprise_stats

      # Test the method works assuming the server responds with the expected
      # json which is mocked in this case from enterprise_status.json
      expect(enterprise_stats.issues.total_issues).to eq(610)
      expect(enterprise_stats.repos.fork_repos).to eq(7)
    end

  end

end

Add the example response from the documentation to spec/fixtures/enterprise_stats.json. I excluded it here since it is rather long.

Run the specs and see that it complains with undefined method enterprise_stats'`.

Now we can open lib/octokit/client/admin_stats.rb. Write the documentation first, then the method.

We add our method to call a get request for the enterprise/stats/all url and it looks something like this.

lib/octokit/client/admin_stats.rb

module Octokit
  class Client
    module AdminStats

      # Get all available enterprise stats
      #
      # @return [Array<Hashie::Mash>] Array of all enterprise stats
      # @see https://support.enterprise.github.com/entries/21188836-admin-stats-api
      # @example Get all enterprise stats
      #   @client.enterprise_status
      def enterprise_stats
        get "enterprise/stats/all"
      end

    end
  end
end

At this point you run the tests and they should all pass. If you had an enterprise install to test against you could run bundle console and test it out for real.

Extracting the get_enterprise_stats(metric) method can come later once everything is in place.

From there it seems like a lot of repetition some times, follow the process of adding the test, mock, doc and method and its smooth sailing.

I want to mention two things I thought about while writing this out. I don't really like the name AdminStats, mostly because it doesn't relate much in the sense that an admin is required to access the information and what we are getting is enterprise stats. We could lump this into the stats module since the naming convention makes it pretty clear were getting enterprise stats. We could also keep it tidy in it's own EnterpriseStats module.

Thanks for taking the time to contribute to Octokit. I know this is quite a bit of reading, mostly for my lack of writing skill and ability to make this more concise but I hope it doesn't deter you.

That's an absolutely fantastic introduction on how to add features to Octokit and is by far one of the best comments on an issue I saw. I really appreciate your time and effort into making this great comment (more like an article). Considering the name, I think that EnterpriseStats is a lot better; I chose AdminStats because I thought there was some special place for Enterprise features.

I will definitely follow your advice and try to implement this feature as soon as I can, I've got a lot of coursework from school ATM! I'm looking forward to working more on Octokit.

Thanks again and I hope you have a great morning/evening!

Also, I think this (or similar to this) should go into the README or the docs; it's über good.

[joeyw]

Closing due to no interest in supporting these.