Bug 17243 - git ignore swagger.min.json
Summary: git ignore swagger.min.json
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-02 21:39 UTC by Benjamin Rokseth
Modified: 2017-06-14 22:11 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17243 - git ignore swagger.min.json (727 bytes, patch)
2016-09-02 21:49 UTC, Benjamin Rokseth
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Rokseth 2016-09-02 21:39:57 UTC
With the advent of the new Koha REST API and recently Bug 16699 and Bug 14868, the minified version of the swagger definitions has snuck into the code.

A bad, but understandable idea. However it will make patching a nightmare, so I propose in this patch to add swagger.min.json to .gitignore and shift the responsibility of minifying elsewhere.

Specific use case: patching two independent bugs on the rest API will fail, as both bugs will need to modify the same swagger.min.json.

Pros: extensible patching on the API will be possible, as was the main bonus of Bug 16699, and is probably going to be the real life use of the API for some time.

Cons: minifySwagger.pl must be run manually, and plack restarted, to pick up changes. This is sysop/deployment responsibility anyways. Alternatively, Bug 17102 can be used to automate minifying.
Comment 1 Benjamin Rokseth 2016-09-02 21:49:53 UTC
Created attachment 55145 [details] [review]
Bug 17243 - git ignore swagger.min.json

This patch adds *.min.json to .gitignore in swagger dir.
To remove tracking of swagger.min.json run
  git update-index --assume-unchanged swagger.min.json

Or just ignore uncommitted changes
Comment 2 Katrin Fischer 2016-09-04 19:35:29 UTC
Could we deal with it similarly as with the OPAC CSS files? (https://wiki.koha-community.org/wiki/Working_with_Bootstrap_OPAC_LESS_files)

I am not sure how this change would affect RM and packaging manager or the end users. Who will generate the minified file and when?
Comment 3 Benjamin Rokseth 2016-09-05 08:55:38 UTC
Well, good question, many possible answers.

Lari proposed in Bug 17102 an alternative to use inotify and a running daemon to pick up changes.

I think, by now, plack should be on by default? At least it is necessary if using the api. Then it would perhaps make sense to build/update the swagger definitions on plack start/restart? It should be an easy patch, since the api has its own builder in plack.psgi.
Comment 4 Benjamin Rokseth 2016-09-05 09:11:15 UTC
Thinking more of it, I would recommend the plack build solution. Something in the line of adding a sub to plack.psgi:

sub minifySwagger {
    use Swagger2;
    my $swagger = Swagger2->new('/usr/share/koha/api/v1/swagger/swagger.json');
    $swagger = $swagger->expand; #Fetch all JSON-Schema references
    my $data = $swagger->api_spec->data;
    delete($data->{id}); # delete non-valid "/id"
    open(SWOUT, ">:encoding(UTF-8)", '/usr/share/koha/api/v1/swagger/swagger.min.json')
    print SWOUT $swagger->to_string();
    close(SWOUT);
}

and calling it inside the build method of the api.

Obviously with some error-handling and relative paths.

Thoughts?
Comment 5 Magnus Enger 2016-09-08 11:28:50 UTC
Looks like the patch submitter is recommending a different solution than the one in the patch? Setting to "In discussion".
Comment 6 Benjamin Rokseth 2016-09-08 11:39:29 UTC
Actually, bug 17102 discusses the alternatives. This patch simply removes the minified swagger from source control, which I think is important. Setting back to need signoff and urge the discussion to move forward in 17102 ;)
Comment 7 Katrin Fischer 2016-09-25 20:19:42 UTC
I've added this bug to the next dev meeting's agenda.
Comment 8 Jonathan Druart 2016-09-29 06:51:37 UTC
We could imagine an "environment" pref (or in the config) with 2 values "dev" or "prod" (default "prod").
If "prod", the minified files are used (otherwise the non-minified).
This could work for the css and swaggger min files.
The RM(aints) will have to regenerated the min fils before each releases (automatically), but nothing to do for devs (no need to regenerated them) nor for testers (no conflict).
Comment 9 Katrin Fischer 2016-09-29 07:12:36 UTC
Hi Jonathan, if that works, I am all for it!
Comment 10 Benjamin Rokseth 2016-09-29 07:40:21 UTC
(In reply to Jonathan Druart from comment #8)
> We could imagine an "environment" pref (or in the config) with 2 values
> "dev" or "prod" (default "prod").
> If "prod", the minified files are used (otherwise the non-minified).
> This could work for the css and swaggger min files.
> The RM(aints) will have to regenerated the min fils before each releases
> (automatically), but nothing to do for devs (no need to regenerated them)
> nor for testers (no conflict).

That is a very sensible suggestion!

Also since it is a general solution that can be used any other place where compilation/minifying/regeneration would be neccessary in the future. Especially in the javascript world and the coming of ES6 this would make sense, as it would cover the need of browser shims and transpilation to code that works across browsers.
Comment 11 Benjamin Rokseth 2016-10-16 13:54:49 UTC
No longer neccessary, due to Bug 17432.