Import OpenAPI v2.0 command #76

eguzki · 2018-11-28T14:33:03Z

import into your 3scale API OpenAPI 2.0 defiinition.

Try to create new service with OpenAPI definition. If system_name already used, update service.

system_name taken from --target_system_name. Default to OpenAPI info.title attribute.

import openapi commad
unittests
integration tests
Documentation in README.md
Optional parameter --target_system_name for specifying system_name
OpenAPI spec can be a file or url
OpenAPI spec can be read from stdin
handle OpenAPI 2.0 parsing/validation errors
command is idempotent

eguzki · 2018-11-30T16:15:44Z

Fixes #38

mikz · 2018-12-03T11:40:25Z

I think this might be the point to start thinking about a dry-run mode. Running potentially huge import with no option to roll back could have a devastating impact on someone's portal.

eguzki · 2018-12-06T12:12:09Z

A couple of questions:

Dry-run mode for this command or for any command doing not read-only operations on remotes?
For the specific case of importing OpenAPI spec case, what strategy do you propose for the implementation? On dry run, mocking 3scale client?

eguzki · 2018-12-11T23:17:57Z

Finished all remaining tasks, PR ready to be reviewed @mikz

mikz · 2018-12-12T09:38:28Z

@eguzki I think dry-run just for this command would be enough. Of course, it would be cool to have it in the others too, but not necessary I'd say.

Regarding mocking the client: I'm not sure if you can mock the client (or you can stub just POST/PUT/DELETE operations). I think the dry mode could be "read only mode". Just read what is the current state and print out the plan (... to be deleted, ... to be changed from .. to ...).

docs/openapi.md

exe/3scale

lib/3scale_toolbox/commands/import_command/openapi/command.rb

mikz · 2018-12-12T09:50:09Z

lib/3scale_toolbox/commands/import_command/openapi/mapping_rule.rb

+          end
+
+          def pattern
+            operation[:path]


Is the pattern compatible with what mapping rules support?

haven't checked that. Is it documented what mapping rules support in pattern field? Send link please.

The documentation: https://github.com/3scale/porta/blob/8022f744c58a8a64f6fcfb8d2f2129fd9eaad74d/config/locales/en.yml#L530-L557

The implementation: https://github.com/3scale/apicast/blob/c3e431df20f65686798d1e67d8c4e9f568ef9c08/gateway/src/apicast/mapping_rule.lua#L30-L35

Mapping rules accepted expression set includes openapi 2.0 path expressions.

The field name MUST begin with a slash and Path Templating is allowed.
https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#patterned-fields

I'm not sure it works.

Defining a path /pets would match /pets/{id} too. Is that desired? Our expressions are not the exact match to the whole string, just matching the beginning.

Cannot find path matching spec in OAS 2.0 spec. I just found clarifications on path overlapping issues

From common sense, I would expect OAS 2.0 spec to do exact match. Just a guess.

Since apicast does just prefix matching, I guess we need to specify somehow end of string when creating mapping rule in 3scale. That only if my assumption about exact match on OAS 2.0 is correct. Is there a way to do that? Adding '/' in the end would not work.

@eguzki it is described in the documentation mentioned in #76 (comment) (using $ at the end)

lib/3scale_toolbox/commands/import_command/openapi/resource_reader.rb

mikz · 2018-12-12T09:54:57Z

lib/3scale_toolbox/commands/import_command/openapi/resource_reader.rb

+          end
+
+          def read_stdin(_resource)
+            content = STDIN.read


I think you can collapse two of these implementations into YAML.parse(ARGF) + handle the URI separately.

First, as mentioned in #76 (comment), we still need to find out format.

Second, ARGF is like cat. Nice but what if we would like to add some extra positional param? Less flexible as far as I see.

Format is not an issue as already mentioned there.

What extra positional arguments? Do you have an example?

It really depends what CRI does to the ARGV and if leaves unprocessed arguments.
That is not as big a deal anyway.

according to https://robots.thoughtbot.com/rubys-argf

ARGF will interpret all elements of the ARGV array as filenames and when read will produce a concatenation of the contents of these files. If ARGV is empty, then ARGF reads from standard input.

Commands and subcommands will be treated as filenames, so hard to integrate with CRI command framework. ARGV needs to be processed first. Moreover, options and flags need to be removed from ARGV before calling ARGF.read.

WRT extra positional argument, imagine we want to pass 3scale instance remote name as positional argument instead of option with -d. It would be treated like filename or it would need to be removed before calling ARGF.read.

IMO unnecessary complication for the provided little value.

spec/shared_contexts.rb

spec/shared_examples.rb

mikz

Excellent work!

There are just a few things I think we have to nail down before merging this:

verify the patterns in the spec match what mapping rules can match
verify the usability of this in a pipeline (creating/updating the same service)
verify what happens hits metric (because it would do double counting of everything)

I don't see any updating to existing mapping rules, so that means I can't run this twice or use it to update the mapping rules after changing the spec. When you plan to address that?

The other comments are mostly related to some possible cleanup.

And of course, the dry-run (read-only) would be pretty nice.

eguzki · 2018-12-12T15:04:56Z

Thanks for the good feedback on relevant issues! The show must go on

…in development

…date, otherwise, create

eguzki · 2018-12-18T15:52:44Z

Verified the patterns in the spec match what mapping rules can match
Update service from system_name. If it does not exist, create it first. system_name can be passed as option parameter and defaults to info.title from openapi spec.
All existing mapping rules are deleted before importing new API definition. Methods left untouched if exist.

eguzki · 2018-12-18T15:53:23Z

rebased to current master, since it diverged too much from branch base

eguzki · 2019-01-14T16:47:48Z

ping @mikz

mikz · 2019-01-16T02:59:39Z

spec/unit/copy_service_spec.rb

@@ -22,6 +22,7 @@
      # Entities::Service instance stub
      service = double('Entities::Service instance')


Would using object double simplify the test, do you don't have to setup id for example?

I do not think it would simplify the test. I would have to construct the service object and pass id and remote objects (doubles?) to the constructor. On the other hand it would check that any stubbed methods on the double also exist on the template, which it is not done now.

I consider this is not a big deal for this test.

Object double verifies that you can't stub a method that does not exist. I think instance_double does the same and does not require the constructor.

eguzki · 2019-01-17T10:52:06Z

regarding dry-run mode, I opened issue on that #87 to leave out of the scope of this PR

eguzki · 2019-01-18T13:04:56Z

applied strict matching on mapping rule patterns
use instance_double when appropriate to check sutbbed methods

eguzki requested a review from mikz November 28, 2018 14:33

eguzki force-pushed the openapi2-import branch 2 times, most recently from ad4adf1 to 27a948e Compare December 10, 2018 11:58

eguzki changed the title ~~(WIP) Import OpenAPI v2.0 command~~ Import OpenAPI v2.0 command Dec 11, 2018