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

WebAPI: Add API description from Controller's POD #1559

Merged
merged 8 commits into from
Feb 13, 2018

Conversation

alvarocarvajald
Copy link
Contributor

@alvarocarvajald alvarocarvajald commented Jan 11, 2018

This PR adds the package OpenQA::WebAPI::Description which contains methods to be used on lib/OpenQA/WebAPI.pm to parse the Controller/API/V1 module files for POD documentation, and extract from there API descriptions to be shown in the GUI's help page.

In order to parse POD, the module Pod::POM was also added to the required modules for the project.

POD documentation was added to three API controller modules:

lib/OpenQA/WebAPI/Controller/API/V1/Feature.pm
lib/OpenQA/WebAPI/Controller/API/V1/JobGroup.pm
lib/OpenQA/WebAPI/Controller/API/V1/Worker.pm
lib/OpenQA/WebAPI/Controller/API/V1/Job.pm
lib/OpenQA/WebAPI/Controller/API/V1/Locks.pm

Adding POD documentation to the remaining modules, should populate API descriptions in the help page automatically.

@okurz
Copy link
Member

okurz commented Jan 12, 2018

wow, that looks very promising. I would like to test that.

@okurz
Copy link
Member

okurz commented Jan 12, 2018

https://build.opensuse.org/request/show/563765 created to provide the dependency for openSUSE but needs perl-HTML-Stream which we don't have. Couldn't test it locally :(

@@ -553,5 +571,110 @@ sub run {
Mojolicious::Commands->start_app('OpenQA::WebAPI');
}

sub get_pod_from_controllers {
Copy link
Member

@foursixnine foursixnine Jan 12, 2018

Choose a reason for hiding this comment

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

I'd suggest to move this to it's own package along with _get_pod_text and related methods so that webAPI has a single responsability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So ... OpenQA::WebAPI::Documentation?

$desc = _get_pod_text($i) if $i->is_ordinary();
}
}
$methods_description{$controller . '#' . $methodname} = $desc;
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to build the srting first, and then use it to avoid perl critic complains: https://travis-ci.org/os-autoinst/openQA/jobs/327749143#L652

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. Will do that.

@alvarocarvajald alvarocarvajald changed the title WebAPI: Add API description from Controller's POD [WIP] WebAPI: Add API description from Controller's POD Jan 12, 2018
@codecov
Copy link

codecov bot commented Jan 12, 2018

Codecov Report

Merging #1559 into master will decrease coverage by 0.23%.
The diff coverage is 61.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1559      +/-   ##
==========================================
- Coverage   88.67%   88.44%   -0.24%     
==========================================
  Files         116      117       +1     
  Lines        8696     8764      +68     
==========================================
+ Hits         7711     7751      +40     
- Misses        985     1013      +28
Impacted Files Coverage Δ
lib/OpenQA/WebAPI/Controller/API/V1/Asset.pm 98.03% <ø> (ø) ⬆️
lib/OpenQA/WebAPI/Controller/API/V1/Job.pm 84.81% <ø> (ø) ⬆️
lib/OpenQA/WebAPI/Controller/API/V1/JobGroup.pm 97.5% <ø> (ø) ⬆️
lib/OpenQA/WebAPI/Controller/API/V1/JobTemplate.pm 76.4% <ø> (ø) ⬆️
lib/OpenQA/WebAPI/Controller/API/V1/Feature.pm 100% <ø> (ø) ⬆️
lib/OpenQA/WebAPI/Controller/API/V1/Comment.pm 100% <ø> (ø) ⬆️
lib/OpenQA/WebAPI/Controller/API/V1/Bug.pm 95.83% <ø> (ø) ⬆️
lib/OpenQA/WebAPI/Controller/API/V1/Mm.pm 56.52% <ø> (ø) ⬆️
lib/OpenQA/WebAPI/Controller/API/V1/Iso.pm 92.91% <ø> (ø) ⬆️
lib/OpenQA/WebAPI/Controller/API/V1/Worker.pm 91.52% <ø> (ø) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3977d2f...70e15bf. Read the comment docs.

@alvarocarvajald
Copy link
Contributor Author

@foursixnine Decided on OpenQA::WebAPI::Description. Let me know if this package name and location is OK.

@@ -0,0 +1,155 @@
# Copyright (C) 2012-2017 SUSE LLC
Copy link
Member

Choose a reason for hiding this comment

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

2018?

@coolo coolo self-requested a review January 23, 2018 11:44
@coolo
Copy link
Contributor

coolo commented Jan 23, 2018

don't merge this before we have a package for pod::tree

@alvarocarvajald
Copy link
Contributor Author

Will investigate Pod::POM as an alternative to Pod::Tree. perl-Pod-POM already exists as a package for opensuse, so if the functionality is equivalent, should be a better option.

@alvarocarvajald
Copy link
Contributor Author

@coolo, @foursixnine, @okurz: reimplemented the POD walking methods in OpenQA::WebAPI::Description to use Pod::POM instead of Pod::Tree. Pod::POM has a package in opensuse, so it should be better to handle dependencies with this one.

@foursixnine
Copy link
Member

@alvarocarvajald Awesome! :) Remember to tidy up too :)

@alvarocarvajald
Copy link
Contributor Author

@foursixnine I did tidy up. :)

From what I can see in my system, files that are complaining on the tidy test are:

./lib/OpenQA/Resource/Jobs.pm
./lib/OpenQA/Schema/Result/Jobs.pm
./lib/OpenQA/WebAPI/Auth/OpenID.pm
./lib/OpenQA/WebAPI/Controller/Admin/JobGroup.pm
./lib/OpenQA/WebAPI/Controller/Admin/User.pm
./lib/OpenQA/WebAPI/Controller/Main.pm
./lib/OpenQA/WebAPI/Controller/Running.pm
./lib/OpenQA/WebAPI/Controller/Test.pm
./lib/OpenQA/WebAPI/Plugin/Gru.pm
./lib/OpenQA/Worker/Commands.pm
./lib/OpenQA/Worker.pm

Which aren't touched by this PR. Should I tidy those as well as part of this PR?

@okurz
Copy link
Member

okurz commented Feb 8, 2018

@coolo did a tidy commit recently. Maybe you just need to rebase?

@coolo
Copy link
Contributor

coolo commented Feb 9, 2018

I guess he uses an old perltidy

This commit adds methods into lib/OpenQA/WebAPI.pm to parse the Controller/API/V1
module files for POD documentation, and extract from there API descriptions to be
shown in the GUI's help page.

In order to parse POD, the module Tree::Pod was also added to the required modules
for the project.
@alvarocarvajald
Copy link
Contributor Author

alvarocarvajald commented Feb 9, 2018

Updated perltidy as suggested and fixed a small issue in lib/OpenQA/WebAPI/Controller/API/V1/Locks.pm. Now it's not reporting any files in my local system ... let's see what travis reports.

@alvarocarvajald alvarocarvajald changed the title [WIP] WebAPI: Add API description from Controller's POD WebAPI: Add API description from Controller's POD Feb 13, 2018
@coolo
Copy link
Contributor

coolo commented Feb 13, 2018

I don't mind merging it - but I consider 34 commits a bit tough - please squash to 1-5 commits

Add POD documentation to Controller/API/V1/Feature.pm

Add POD documentation to Controller/API/V1/JobGroup.pm

Add POD documentation to Controller/API/V1/Worker.pm
WebAPI: Remove uses of $_[0] in support methods

WebAPI: Build file#method key before use in hash

WebAPI: Move Pod::Tree->set_filename call

Move call to Pod::Tree->set_filename in get_pod_from_controllers()
to avoid warnings when the file is not loaded

Move methods used to parse POD from API controller modules and to set
API descriptions to OpenQA::WebAPI::Description.

Remove unnecessary call to log_warning

Fix copyright date

Verify module file existance before parsing

Change if for unless in OpenQA::WebAPI::Description

To improve readibility
Add POD documentation to Controller/API/V1/Job.pm

Add POD documentation to Controller/API/V1/Locks.pm
This commits reimplements the OpenQA::WebAPI::Description
package to use Pod::POM instead of Pod::Tree.

The file cpanfile with the required modules was also changed
to reflect this.

Tidy lib/OpenQA/WebAPI.pm

Tidy lib/OpenQA/WebAPI/Controller/API/V1/Locks.pm
t/27-errorpages.t: Update API description to check

Due to changes from commit os-autoinst@fd50a10, API descriptions
are taken from the POD documentation in the Controller/API/V1/*.pm files; as such
the text used to check the description in t/27-errorpages.t needs to be updated
accordingly.

Change the API description string to check.

Fix test text in t/27-errorpages.t

Better test string in t/27-errorpages.t

Tidy t/27-errorpages.t
Add POD documentation Controller/API/V1/Asset.pm

Add POD documentation to Controller/API/V1/Bug.pm

Add POD documentation to Controller/API/V1/Command.pm

Add POD documentation to Controller/API/V1/Comment.pm

Add POD documentation to Controller/API/V1/Iso.pm

Add description for /api/v1 root api path

Add POD documentation to Controller/API/V1/JobTemplate.pm

Add POD documentation for Controller/API/V1/Mm.pm

Add METHODS heading in Controller/API/V1/* PODs

Add POD documentation to Controller/API/V1/Table.pm
@alvarocarvajald
Copy link
Contributor Author

Squashed to 8 to keep some order between the different commits. Hope that's ok.

@coolo
Copy link
Contributor

coolo commented Feb 13, 2018

ok, fair compromise :)

@coolo coolo merged commit 8d54de2 into os-autoinst:master Feb 13, 2018
coolo pushed a commit that referenced this pull request Feb 13, 2018
commit 8d54de2
Merge: 3977d2f 70e15bf
Author:     Stephan Kulow <stephan@kulow.org>
AuthorDate: Tue Feb 13 21:49:09 2018 +0100
Commit:     GitHub <noreply@github.com>
CommitDate: Tue Feb 13 21:49:09 2018 +0100

    Merge pull request #1559 from alvarocarvajald/poo#16282

    WebAPI: Add API description from Controller's POD
@alvarocarvajald alvarocarvajald deleted the poo#16282 branch February 20, 2018 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants