MG-1962 - Add single endpoint to change status of domain #2127
Conversation
WashingtonKK
changed the title
janael-pinheiro
force-pushed
the
MG-1962
branch
from
c8fe404 to
417666b
Compare
|
@dborovcanin and @arvindh123 please review this one |
| /domains/{domainID}/disable: | ||
| post: | ||
| summary: Disable a domain | ||
| /domains/{domainID}/status: |
There was a problem hiding this comment.
Why is important to have one endpoint? It looks more self-documenting to me that way, than having sing status endpoint - which usually prompt me that it is status indicator (i.e. something I would use with GET to fetch the current status), and not a control switch.
| @@ -349,6 +349,25 @@ func (lm *loggingMiddleware) CreateDomain(ctx context.Context, token string, d a | |||
| return lm.svc.CreateDomain(ctx, token, d) | |||
| } | |||
|
|
|||
| func (lm *loggingMiddleware) DeleteDomain(ctx context.Context, token string, d auth.Domain) (err error) { | |||
There was a problem hiding this comment.
Probably deletion was avoided for now because of cascade consequences - @dborovcanin must confirm. But probably you'll need to remove all resources related to this domain (things, channels, ...) once you decide to remove a specific domain.
There was a problem hiding this comment.
@drasko This is correct, it requires a careful analysis due to the cascade nature of removal.
janael-pinheiro
force-pushed
the
MG-1962
branch
from
417666b to
083d67a
Compare
|
@arvindh123 Please review this one. |
dborovcanin
force-pushed
the
MG-1962
branch
2 times, most recently
from
9f58fc1 to
1cb281f
Compare
Signed-off-by: Janael Pinheiro <ajp@cesar.org.br>
|
After this comment and an internal discussion between the maintainer and due to the inactivity of this pull request, we decided to close this PR and linked issue, since we will keep the current approach. Both approaches diverge from the usual REST principle, the current one (with 3 endpoints) being less prone to errors and a little more verbose. @janael-pinheiro Thanks for the contribution and effort you put into this pull request. |
What type of PR is this?
This is a feature because it adds the following functionality: adds a single endpoint to change domain status and an endpoint to remove domain.
What does this do?
removes the
/domains/{domainID}/enabled,/domains/{domainID}/disabled, and/domains/{domainID}/freezedendpoints. Adds the/domains/{domainID}/status(HTTP PUT) endpoint that concentrates domain status change operations. Additionally, it adds the/domains/{domainID}(HTTP DELETE) endpoint to remove a specific domain.Which issue(s) does this PR fix/relate to?
Have you included tests for your changes?
Yes, I have included tests for my changes. I added tests to evaluate the behavior of the new endpoints (
/domains/{domainID}/statusand/domains/{domainID}).Did you document any new/modified feature?
Yes, I have updated the documentation for the new feature. Yes, I updated the
api/openapi/auth.ymlfileNotes