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

Get all resource servers #155

Merged
merged 4 commits into from
Jan 23, 2019

Conversation

philomory
Copy link
Contributor

@philomory philomory commented Jan 19, 2019

Changes

Adds Auth0::Api::V2::ResourceServers#resource_servers method (and alias get_resource_servers) to fetch all Resource Servers in the tenant.

References

Discussion in #154

Testing

Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.

  • This change adds unit test coverage
  • This change adds integration test coverage
  • This change has been tested on the latest version of Ruby
    (my testing was done on 2.4.4, not 2.6.0)

Checklist

  • I have read the Auth0 general contribution guidelines
  • I have read the Auth0 Code of Conduct
  • All existing and new tests complete without errors - specs pass, I can't run integration tests locally
  • Rubocop passes on all added/modified files - they don't pass on master when I run them, either
  • All active GitHub checks have passed

Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

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

@philomory - Looks great! Thank you for this ... couple of minor comments here.

Also, I recorded VCR cassettes for the integration tests. If you can commit these as well, I can merge this:

https://www.dropbox.com/s/tx9vnfksqjxiuvx/Auth0_Api_V2_ResourceServers.zip?dl=0

Drop these in spec/fixtures/vcr_cassettes and CI tests should complete with HTTP calls turned off.

per_page: 1
)
expect(results.first).to equal(results.last)
expect(results.first).to eq(test_server)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not necessary, and is failing on my end when I run the integration tests on your branch. We're not pulling by any specific order here so the newest may (or may not) be the first one pulled.

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've removed the expect(results.first).to eq(test_server) line, which I believe is the only one you objected to. If you want me to remove that whole test about paging, though, I can do that instead.

@@ -22,7 +22,7 @@
VCR.configure do |config|
# Uncomment the line below to record new VCR cassettes.
# When this is commented out, VCR will reject all outbound HTTP calls.
# config.allow_http_connections_when_no_cassette = true
config.allow_http_connections_when_no_cassette = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to comment this back out so HTTP calls are not made.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's been commented-out again.

Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks again for sticking with this.

@joshcanhelp joshcanhelp merged commit b9691c4 into auth0:master Jan 23, 2019
@joshcanhelp joshcanhelp added this to the v4.7.0 milestone Jan 23, 2019
@philomory philomory deleted the get_all_resource_servers branch April 9, 2019 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants