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

REST API #1299

Merged
merged 1 commit into from Aug 14, 2017

Conversation

Projects
None yet
3 participants
@Vad1mo
Contributor

Vad1mo commented Jun 9, 2017

REST API for Portus to manage Users and Tokens. Furthermore based on this effort here it is possible to add more APIs to Portus.

PR Content and Features:

  • CRUD Users and their application tokens via a REST PAI
  • Swagger Spec/OpenAPI
  • Documentation
@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola Jun 10, 2017

Contributor

First of all, thanks a lot for this 👍

Now, you may want to take a look at what we are doing here: #1289. There's a lot of stuff on this PR, but take into consideration:

  • The usage of PORTUS-AUTH in app/controllers/application_controller.rb. This is inspired by what Gitlab is doing, and it allows remote clients to make API calls.
  • The app/controllers/tags_controller.rb file now implements the show method, which is the first one providing API support.

Now, this is far from perfect, so I encourage you to innovate upon these ideas. I also thought of using something like grape, but so far I've used good ol' Rails as I'm already used to it (also notice all the ugly as_json methods, that I'd like to get rid off with a proper serializer or other solution...).

Contributor

mssola commented Jun 10, 2017

First of all, thanks a lot for this 👍

Now, you may want to take a look at what we are doing here: #1289. There's a lot of stuff on this PR, but take into consideration:

  • The usage of PORTUS-AUTH in app/controllers/application_controller.rb. This is inspired by what Gitlab is doing, and it allows remote clients to make API calls.
  • The app/controllers/tags_controller.rb file now implements the show method, which is the first one providing API support.

Now, this is far from perfect, so I encourage you to innovate upon these ideas. I also thought of using something like grape, but so far I've used good ol' Rails as I'm already used to it (also notice all the ugly as_json methods, that I'd like to get rid off with a proper serializer or other solution...).

@Vad1mo

This comment has been minimized.

Show comment
Hide comment
@Vad1mo

Vad1mo Jun 10, 2017

Contributor

I saw some of the work in the branch. We will take PORTUS-AUTH into consideration. @mssola when do you plan to merge #1289 into master?
I like the grape approach. We also see this as an viable option and thinking about using it.

Contributor

Vad1mo commented Jun 10, 2017

I saw some of the work in the branch. We will take PORTUS-AUTH into consideration. @mssola when do you plan to merge #1289 into master?
I like the grape approach. We also see this as an viable option and thinking about using it.

@Vad1mo

This comment has been minimized.

Show comment
Hide comment
@Vad1mo

Vad1mo Jun 10, 2017

Contributor

It would be nice if someone could review the API Docs f1999b7 It would be nice if @mssola or someone else would take some time to review the API concept.

Contributor

Vad1mo commented Jun 10, 2017

It would be nice if someone could review the API Docs f1999b7 It would be nice if @mssola or someone else would take some time to review the API concept.

@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola Jun 12, 2017

Contributor

I saw some of the work in the branch. We will take PORTUS-AUTH into consideration. @mssola when do you plan to merge #1289 into master?

Both me and @vitoravelino have been busy with other stuff, and it will probably take a while, since we have to review the code, and apply more testing... I'd say I can break away the code for managing PORTUS-AUTH in another PR, so you can use it.

I like the grape approach. We also see this as an viable option and thinking about using it.

I have to admit that I've never used it, so this is something I'm not really sure. It needs some exploration from my part 😄

It would be nice if someone could review the API Docs f1999b7 It would be nice if @mssola or someone else would take some time to review the API concept.

Left some comments. In principle it looks good to me 👍

Contributor

mssola commented Jun 12, 2017

I saw some of the work in the branch. We will take PORTUS-AUTH into consideration. @mssola when do you plan to merge #1289 into master?

Both me and @vitoravelino have been busy with other stuff, and it will probably take a while, since we have to review the code, and apply more testing... I'd say I can break away the code for managing PORTUS-AUTH in another PR, so you can use it.

I like the grape approach. We also see this as an viable option and thinking about using it.

I have to admit that I've never used it, so this is something I'm not really sure. It needs some exploration from my part 😄

It would be nice if someone could review the API Docs f1999b7 It would be nice if @mssola or someone else would take some time to review the API concept.

Left some comments. In principle it looks good to me 👍

@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola Jun 21, 2017

Contributor

@Vad1mo ping me whenever this is ready to be reviewed.

Contributor

mssola commented Jun 21, 2017

@Vad1mo ping me whenever this is ready to be reviewed.

@Vad1mo

This comment has been minimized.

Show comment
Hide comment
@Vad1mo

Vad1mo Jun 21, 2017

Contributor

@mssola we are fixing the travis build and adding some documentation next to the swagger spec. The code is basically ready for an review.

fyi: you can get the swagger spec localhost:3000/api/swagger_doc

Contributor

Vad1mo commented Jun 21, 2017

@mssola we are fixing the travis build and adding some documentation next to the swagger spec. The code is basically ready for an review.

fyi: you can get the swagger spec localhost:3000/api/swagger_doc

@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola Jun 21, 2017

Contributor

@Vad1mo ok, I'll wait also for the swagger documentation if you don't mind 😉

Contributor

mssola commented Jun 21, 2017

@Vad1mo ok, I'll wait also for the swagger documentation if you don't mind 😉

@andrew2net

This comment has been minimized.

Show comment
Hide comment
@andrew2net

andrew2net Jun 22, 2017

@mssola Hello,
I'm trying to pass travis. Running command, which got an error on travis:

bundle exec rspec spec packaging/suse/portusctl/spec

got

...
rpc error: code = 2 desc = oci runtime error: exec failed: container_linux.go:247: starting container process caused "exec: \"mysqladmin\": executable file not found in $PATH"

Command 'mysqladmin --count=1 --connect-timeout=1 --host=integration_db -u root --password=portus ping' failed! Waiting 5 seconds...
...

can you help me find cause?

andrew2net commented Jun 22, 2017

@mssola Hello,
I'm trying to pass travis. Running command, which got an error on travis:

bundle exec rspec spec packaging/suse/portusctl/spec

got

...
rpc error: code = 2 desc = oci runtime error: exec failed: container_linux.go:247: starting container process caused "exec: \"mysqladmin\": executable file not found in $PATH"

Command 'mysqladmin --count=1 --connect-timeout=1 --host=integration_db -u root --password=portus ping' failed! Waiting 5 seconds...
...

can you help me find cause?

@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola Jun 22, 2017

Contributor

@andrew2net have you tried without running the integration tests ? (they might be broken...):

$ bundle exec rspec spec packaging/suse/portusctl/spec --tag ~integration
Contributor

mssola commented Jun 22, 2017

@andrew2net have you tried without running the integration tests ? (they might be broken...):

$ bundle exec rspec spec packaging/suse/portusctl/spec --tag ~integration
@andrew2net

This comment has been minimized.

Show comment
Hide comment
@andrew2net

andrew2net Jun 22, 2017

@mssola I haven't. traied now and all tests passed. Does it need to pass integration tests for successfull travis checkung?

andrew2net commented Jun 22, 2017

@mssola I haven't. traied now and all tests passed. Does it need to pass integration tests for successfull travis checkung?

@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola Jun 22, 2017

Contributor

@andrew2net no. Travis does not run integration tests for Portus.

Contributor

mssola commented Jun 22, 2017

@andrew2net no. Travis does not run integration tests for Portus.

@Vad1mo

This comment has been minimized.

Show comment
Hide comment
@Vad1mo

Vad1mo Jun 22, 2017

Contributor

@mssola you can take a look now at the PR

Contributor

Vad1mo commented Jun 22, 2017

@mssola you can take a look now at the PR

@mssola

Left some comments. Thanks a lot for all your hard work 👏

Show outdated Hide outdated .gitignore Outdated
@@ -29,6 +29,9 @@ Style/Documentation:
Lint/UnneededDisable:
Enabled: false
Style/FrozenStringLiteralComment:
Enabled: false

This comment has been minimized.

@mssola

mssola Jun 23, 2017

Contributor

This wasn't an issue before 😕 If you call it with bundle exec there shouldn't be any problem no ?

@mssola

mssola Jun 23, 2017

Contributor

This wasn't an issue before 😕 If you call it with bundle exec there shouldn't be any problem no ?

This comment has been minimized.

@andrew2net

andrew2net Jun 23, 2017

@mssola with ruby 2.3 rubocop reports offense Missing frozen string literal comment. on each rb file

@andrew2net

andrew2net Jun 23, 2017

@mssola with ruby 2.3 rubocop reports offense Missing frozen string literal comment. on each rb file

This comment has been minimized.

@mssola

mssola Jun 26, 2017

Contributor

Gotcha

@mssola

mssola Jun 26, 2017

Contributor

Gotcha

@@ -1 +1 @@
2.1.2
2.3.3

This comment has been minimized.

@mssola

mssola Jun 23, 2017

Contributor

Unrelated to this PR.

@mssola

mssola Jun 23, 2017

Contributor

Unrelated to this PR.

This comment has been minimized.

@Vad1mo

Vad1mo Jun 23, 2017

Contributor

well, grape requires to go to minimum 2.3.

@Vad1mo

Vad1mo Jun 23, 2017

Contributor

well, grape requires to go to minimum 2.3.

This comment has been minimized.

@mssola

mssola Jun 26, 2017

Contributor

I'll have to check about this requirement...

@mssola

mssola Jun 26, 2017

Contributor

I'll have to check about this requirement...

Show outdated Hide outdated .travis.yml Outdated
Show outdated Hide outdated Gemfile Outdated
Show outdated Hide outdated app/controllers/vulnerabilities_controller.rb Outdated
Show outdated Hide outdated app/api/grape_api/user_api.rb Outdated
Show outdated Hide outdated app/api/grape_api/root_api.rb Outdated
Show outdated Hide outdated app/api/grape_api/root_api.rb Outdated
Show outdated Hide outdated app/api/grape_api/root_api.rb Outdated
@mssola

This comment has been minimized.

Show comment
Hide comment
@mssola

mssola Jul 18, 2017

Contributor

Note that we have just merged #1289, and so there are quite some conflicts now. Also, make sure to rebase against master.

Contributor

mssola commented Jul 18, 2017

Note that we have just merged #1289, and so there are quite some conflicts now. Also, make sure to rebase against master.

@Vad1mo Vad1mo changed the title from WIP: REST API to REST API Jul 19, 2017

@Vad1mo

This comment has been minimized.

Show comment
Hide comment
@Vad1mo

Vad1mo Jul 20, 2017

Contributor

@mssola IMHO, we are ready now, for the review.

Contributor

Vad1mo commented Jul 20, 2017

@mssola IMHO, we are ready now, for the review.

@mssola mssola added this to the Release 2.3 milestone Aug 2, 2017

@mssola

In general it looks good to me. Just fix my comments 😉 I still have to test it locally more deeply though ...

Show outdated Hide outdated app/controllers/concerns/auth_from_token.rb Outdated
@@ -0,0 +1,16 @@
module AuthFromToken

This comment has been minimized.

@mssola

mssola Aug 4, 2017

Contributor

Please, add documentation

@mssola

mssola Aug 4, 2017

Contributor

Please, add documentation

Show outdated Hide outdated app/controllers/concerns/auth_from_token.rb Outdated
Show outdated Hide outdated lib/api/root_api.rb Outdated
Show outdated Hide outdated lib/api/root_api.rb Outdated
Show outdated Hide outdated lib/api/v1/user_api.rb Outdated
Show outdated Hide outdated lib/api/v1/user_api.rb Outdated
Show outdated Hide outdated lib/api/v1/user_api.rb Outdated
Show outdated Hide outdated lib/api/v1/user_api.rb Outdated
Show outdated Hide outdated lib/api/v1/user_api.rb Outdated
@mssola

Some comments after using it:

There should be a way to return a 404 when the path does not exist. For example I typed /api/users, and I got an HTML page, instead of simply a not found error. Maybe in production this is already handled?

Besides this, when I performed:

curl -X DELETE \
     -H "PORTUS-AUTH: mssola:KXsgWZQyDq8rMoy-hHaD" \
    https://registry.mssola.cat/api/v1/users/2/application_tokens/2

I got:

{
  "error": "Internal server error: Grape::Exceptions::MethodNotAllowed"
}

Overall really nice work. Thanks a lot for your patience 👏

Show outdated Hide outdated lib/api/root_api.rb Outdated
Show outdated Hide outdated lib/api/root_api.rb Outdated
Show outdated Hide outdated lib/api/v1/users.rb Outdated
Show outdated Hide outdated lib/api/v1/users.rb Outdated
@andrew2net

This comment has been minimized.

Show comment
Hide comment
@andrew2net

andrew2net Aug 8, 2017

@mssola API with GRAPE processes requests if the path matches any of endpoints. If not then the request is ignored by GRAPE and the app processes it and returns No route matches error in development as HTML if route not exists.
Will add interception of Grape::Exceptions::MethodNotAllowed error.

andrew2net commented Aug 8, 2017

@mssola API with GRAPE processes requests if the path matches any of endpoints. If not then the request is ignored by GRAPE and the app processes it and returns No route matches error in development as HTML if route not exists.
Will add interception of Grape::Exceptions::MethodNotAllowed error.

@mssola

If I submit a malformed JSON (create user), I receive and HTML. It should instead return a proper error response (in this case this is a 400 Bad Request).

We are getting really close, nice work!

Show outdated Hide outdated spec/api/grape_api/v1/users_spec.rb Outdated
@mssola

LGTM

Rebase all this to the master branch, and squash commits into a set of commits (or a single commit even)

Again, thanks a lot for all this. You rock 👏

Portus REST API for creating and managing users and tokens.
Signed-off-by: Vadim Bauer <bauer.vadim@gmail.com>
@mssola

mssola approved these changes Aug 14, 2017

LGTM 👏

@mssola mssola merged commit 586636b into SUSE:master Aug 14, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mssola mssola referenced this pull request Aug 14, 2017

Merged

js: Namespaces#index refactoring w/ Vue #1371

8 of 8 tasks complete

@mssola mssola referenced this pull request Nov 2, 2017

Open

Consolidate the REST API #1500

@Vad1mo Vad1mo deleted the Vad1mo:api branch Feb 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment