Skip to content

Reinstate old RPC API methods#1111

Merged
13 commits merged into
developfrom
unknown repository
Jun 14, 2023
Merged

Reinstate old RPC API methods#1111
13 commits merged into
developfrom
unknown repository

Conversation

@ghost

@ghost ghost commented Jun 7, 2023 •
edited by ghost
Loading

Copy link
Copy Markdown

Purpose

Remove deprecation warning and comments to all old RPC API methods and mark the new methods as experimental.
What were the deprecated methods (and old stable) are again the default stable API methods.

Context

zonemaster/zonemaster#1159

Relates to #1054 and #1083.

Changes

Revert commits where old methods were deprecated and based upon new methods. Now the experimental methods are based on the stable one.

How to test this PR

  • Unit tests should pass
  • Review the code and verify that the unit tests cover the stable API
  • Check that the new experimental API is working (there is currently no unit test)
  • Check that changes made in Backend for this release are still working (and have not been inadvertently reverted)

@ghost ghost added this to the v2023.1 milestone Jun 7, 2023
@ghost ghost requested review from hannaeko, marc-vanderwal, matsduf, mattias-p and tgreenx June 7, 2023 15:10
@ghost ghost marked this pull request as ready for review June 7, 2023 15:25
@ghost ghost marked this pull request as draft June 7, 2023 15:56
@matsduf

matsduf commented Jun 7, 2023

Copy link
Copy Markdown
Contributor

share/backend_config.ini also needs to be restored.

Alexandre Pion added 9 commits June 9, 2023 11:38
Revert "Deprecate RPCAPI config properties"
b1093b7 
This reverts commit 86063c6.

Skip a now empty unit test on deprecated configuration values.
Revert "Update scripts with new RPCAPI methods"
671dcf5 
This reverts commit 41850d0.
Revert "Update unit tests with new RPCAPI methods"
b7a2806 
This reverts commit 87a2220.
Revert "Reuse new API methods for deprecate one"
317a4ab 
This reverts commit 4dd6d09.
Revert "Refactor: reuse old API methods"
5a20745 
This reverts commit c619bec.
Remove deprecated comments
113e320 
* Revert "Mark all API methods as deprecated"
  This reverts commit 5169feb.
* Remove other comments from other commits
Mark new methods as "Experimental"
65c9910
Reuse API methods for new experimental methods
c1fc05f 
* Some API methods are already returning an object. This object can be
  returned.
* For other methods, wrap the result in an object
Refactor: reorder API methods
a3004f8
@ghost ghost marked this pull request as ready for review June 9, 2023 09:48
@ghost

ghost commented Jun 9, 2023

Copy link
Copy Markdown
Author

share/backend_config.ini also needs to be restored.

Good point. I've restarted from scratch this PR with an hopefully better approach allowing for a better understanding of what's being done. I think this is now ready for review.

mattias-p
mattias-p previously approved these changes Jun 9, 2023
View reviewed changes

@mattias-p mattias-p left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I haven't tested but this looks good to me.

@ghost ghost mentioned this pull request Jun 12, 2023
Merged
@mattias-p mattias-p self-requested a review June 12, 2023 09:48
tgreenx
tgreenx previously approved these changes Jun 12, 2023
View reviewed changes

@tgreenx tgreenx left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM while doing a manual, visual comparison. Testing remains to be done.

Just one non-blocking comment.

Comment thread t/config.t
@mattias-p

mattias-p commented Jun 12, 2023

Copy link
Copy Markdown
Member

In the psgi file there's a check for $ini->RPCAPI_enable_user_create or $ini->RPCAPI_enable_add_batch_user. Since RPCAPI.enable_add_batch_user is truthy by default, the only way to disable the methods is to disable RPCAPI.enable_add_batch_user (and leave RPCAPI.enable_user_create disabled), right?

Also from my point of view it would be better to forbid specifying both RPCAPI.enable_add_batch_user and RPCAPI.enable_user_create in the same config.

Alexandre Pion added 2 commits June 12, 2023 15:37
Add configuration unit test for RPCAPI section
9e75a9f
Set experimental config parameters as aliases
3c95b9d 
* using RPCAPI.enable_add_api_user or RPCAPI.enable_user_create should
  enable both add_api_user and user_create methods.
* using RPCAPI.enable_add_batch_job or RPCAPI.enable_batch_create should
  enable both add_batch_job and batch_create methods.
* add configuration unit test
@ghost ghost dismissed stale reviews from tgreenx and mattias-p via 3c95b9d June 12, 2023 15:29
@ghost

ghost commented Jun 12, 2023

Copy link
Copy Markdown
Author

I've updated the PR to be aligned with the current documentation in zonemaster/zonemaster#1162. And as suggested, an error is now raised if both stable and experimental parameters are defined in the configuration file.

@ghost ghost requested a review from tgreenx June 12, 2023 15:32
tgreenx
tgreenx requested changes Jun 13, 2023
View reviewed changes

@tgreenx tgreenx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have manually tested all methods ("old" and "new") and found one error. Other than that LGTM.

Comment thread lib/Zonemaster/Backend/RPCAPI.pm Outdated
tgreenx
tgreenx previously approved these changes Jun 13, 2023
View reviewed changes
mattias-p
mattias-p previously approved these changes Jun 13, 2023
View reviewed changes

@mattias-p mattias-p left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

Comment thread t/test01.t Outdated
@ghost ghost dismissed stale reviews from mattias-p and tgreenx via ed46194 June 14, 2023 08:06
@ghost ghost requested review from mattias-p and tgreenx June 14, 2023 08:06
@ghost

ghost commented Jun 14, 2023

Copy link
Copy Markdown
Author

To quote comment from the documentation PR (zonemaster/zonemaster#1162 (comment)):

AFAICT this PR is done. I've gotten a few reviews, addressed all comments up to this point and everybody seems ok with the implementation.

I'm leaving it open until the implementation is done too so they can be merged at the same time.

I think we reached that point.

@matsduf

matsduf commented Jun 14, 2023

Copy link
Copy Markdown
Contributor

I think this can be merged.

@ghost ghost merged commit 92a19ed into zonemaster:develop Jun 14, 2023
@ghost ghost deleted the rpcapi-revert-and-experimental branch June 14, 2023 13:12
@mattias-p mattias-p mentioned this pull request Jul 8, 2023
Open
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matsduf matsduf Awaiting requested review from matsduf
@hannaeko hannaeko Awaiting requested review from hannaeko
@marc-vanderwal marc-vanderwal Awaiting requested review from marc-vanderwal

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2023.1

Development

Successfully merging this pull request may close these issues.

3 participants

@matsduf @mattias-p @tgreenx