Skip to content

Postpone stabilization of new RPCAPI methods and config properties#1162

Merged
mattias-p merged 5 commits into
zonemaster:developfrom
mattias-p:1159-postpone-stabilization
Jun 14, 2023
Merged

Postpone stabilization of new RPCAPI methods and config properties#1162
mattias-p merged 5 commits into
zonemaster:developfrom
mattias-p:1159-postpone-stabilization

Conversation

@mattias-p

@mattias-p mattias-p commented Jun 9, 2023

Copy link
Copy Markdown
Member

Purpose

This PR re-stabilizes the old RPCAPI methods and config properties and marks the new ones as experimental.

Context

Fixes #1159.

Changes

  • Re-stabilize the old RPCAPI methods.
  • Mark the new RPCAPI methods as experimental.
  • Reinstate references to enable_add_api_user and enable_add_batch_job.
  • Re-stabilize the old config properties.
  • Characterize the new config properties as experimental aliases.

How to test this PR

It would make sense to check the following things:

  • Enable and disable the following configuration properties and make sure they behave as documented.
    • RPCAPI.enable_add_api_user
    • RPCAPI.enable_add_batch_job
    • RPCAPI.enable_user_create
    • RPCAPI.enable_batch_create
  • Make calls to all the old RPCAPI methods and make sure they behave as documented.

mattias-p added 3 commits June 9, 2023 12:10
@mattias-p
Tidy
665fbd7
@mattias-p
Move back to old RPCAPI methods
ebd37eb 
* Re-stabilize the old RPCAPI methods.
* Mark the new RPCAPI methods as experimental.
* Reinstate references to enable_add_api_user and enable_add_batch_job.
@mattias-p
Move back to old config properties
2b142d5 
* Re-stabilize the old config properties.
* Characterize the new config properties as experimental aliases.
@mattias-p mattias-p added this to the v2023.1 milestone Jun 9, 2023
@mattias-p mattias-p requested review from a user, hannaeko, marc-vanderwal, matsduf and tgreenx June 9, 2023 11:15
ghost
ghost previously approved these changes Jun 12, 2023
View reviewed changes

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This looks fine. Just a non blocking remark.

Comment thread docs/public/configuration/backend.md

If defined, the [enable_batch_create][RPCAPI.enable_batch_create]
property takes precedence over this.
An experimental alias for [enable_add_batch_job][RPCAPI.enable_add_batch_job].

@ghost ghost Jun 12, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good idea to make it an alias. I'm not sure I've implemented it that way in zonemaster/zonemaster-backend#1111. I totally can update zonemaster/zonemaster-backend#1111 to make it an alias. Should I wait for this PR to be merged or approved to do that?

@tgreenx tgreenx Jun 12, 2023

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 would say so yes.

@mattias-p mattias-p Jun 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I thought it was your good idea to make it an alias :) That's how I read zonemaster/zonemaster-backend#1111. But now that I looked at it again I found a couple of flaws in this interpretation.

  1. They have different default values.
  2. When you specify RPCAPI.enable_add_batch_job twice you get an error, but if you specify both RPCAPI.enable_add_batch_job and RPCAPI.enable_user_create the maximum value takes precedence.

Merging either the interface or the implementation before the other exposes us to the risk of having to go back. Ideally I think the interface and the implementation should be merged in the same PR, but now they're in separate repositories, so I guess the next best thing is to leave both open until both are sufficiently approved.

tgreenx
tgreenx reviewed Jun 12, 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.

Two comments otherwise LGTM.

Comment thread docs/public/configuration/backend.md

If defined, the [enable_batch_create][RPCAPI.enable_batch_create]
property takes precedence over this.
An experimental alias for [enable_add_batch_job][RPCAPI.enable_add_batch_job].

@tgreenx tgreenx Jun 12, 2023

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 would say so yes.

Comment thread docs/public/using/backend/rpcapi-reference.md
Comment thread docs/public/using/backend/rpcapi-reference.md
@mattias-p mattias-p force-pushed the 1159-postpone-stabilization branch from 27f4fbd to e68ba0c Compare June 12, 2023 13:00
tgreenx
tgreenx previously approved these changes Jun 12, 2023
View reviewed changes
@mattias-p

mattias-p commented Jun 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

AFAICT this PR is done. I've gotten a few reviews, addressed all comments up to this point and everybody seems to like the "alias" line.

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

But don't be afraid to add more reviews or discussion on the design. I'll address those as well.

@ghost ghost mentioned this pull request Jun 12, 2023
Merged
@mattias-p mattias-p force-pushed the 1159-postpone-stabilization branch from 713663b to f649f13 Compare June 13, 2023 08:08
@mattias-p

mattias-p commented Jun 13, 2023

Copy link
Copy Markdown
Member Author

I found a couple more broken links related to the ones @tgreenx pointed out, so I pushed another commit for those.

Please review the last commit. Sorry for the inconvenience.

I also found a whole bunch of other broken links, but I'm deferring those to a separate PR.

@mattias-p mattias-p force-pushed the 1159-postpone-stabilization branch from f649f13 to e73b998 Compare June 13, 2023 08:18
@tgreenx tgreenx linked an issue Jun 13, 2023 that may be closed by this pull request
Postpone stabilization of new RPC API methods #1159
Closed
@mattias-p mattias-p merged commit e3c65d7 into zonemaster:develop Jun 14, 2023
@tgreenx tgreenx mentioned this pull request Aug 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tgreenx tgreenx tgreenx approved these changes
@matsduf matsduf Awaiting requested review from matsduf
@hannaeko hannaeko Awaiting requested review from hannaeko
@marc-vanderwal marc-vanderwal Awaiting requested review from marc-vanderwal
+1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2023.1

Development

Successfully merging this pull request may close these issues.

Postpone stabilization of new RPC API methods

2 participants

@mattias-p @tgreenx