-
Notifications
You must be signed in to change notification settings - Fork 1
Feature/cf 1190 introduce quote endpoint #208
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
Conversation
|
PR Packages Published Python Package:
NPM Package:
|
|
The latest Buf updates on your PR. Results from workflow Pull Request / linting (pull_request).
|
fe6a33e to
eda6b6e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the PR doesn't include updates for the generated code.
Please make sure you've run the following commands from the root directory:
make generateOnce you've added them, you can dismiss this review.
eda6b6e to
39412da
Compare
| // This endpoint calculates pricing information including hourly and monthly costs, | ||
| // and any applicable discounts for the specified cluster configuration. | ||
| // Required permissions: | ||
| // - None (authenticated only) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO We should have a proper permission, I don't like we leak endpoints which in enterprises should not be able to be called unless allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the api requires authentication but does not require the permission.
I would go with public api - without authentication and permission. It can be useful for calculator page for example.
or If we want it to be authenticated, then we should add a permission for it.
Since the response may has info about discounts, I would go with authenticated and permission checked api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does it sound if I add a read:discounts permission, and if the user has it he will see the discounted price, if not he will see the prices with zero discount
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I drafted the following implementation:
• The getquote endpoint will be public in general, but discounts will only be applied if the user has the read:discount permission. Otherwise, the user will see the quote without discounts, even if they exist.
After discussing this with the team, a few concerns were raised:
• Some felt it’s not clean to have different behavior in the same endpoint depending on permissions. An alternative would be to expose two separate endpoints (getquote and getquote_withdiscounts), letting the client decide based on permissions. At the very least, the response should make it explicit whether no discounts exist versus the user not being allowed to see them.
• Others argued that permissions might not be the right tool for this, and that a more fine-grained ABAC approach would be preferable (though we don’t have such a system in place yet).
• It was also mentioned that we could start with no permissions for this at all.
cc @pansen @Rendez
What do you think @Robert-Stam @lomidzemikheili?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New update, we believe that if the user has the write to create cluster he should see the price including any discounts
lomidzemikheili
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Left two comments
| // This endpoint calculates pricing information including hourly and monthly costs, | ||
| // and any applicable discounts for the specified cluster configuration. | ||
| // Required permissions: | ||
| // - None (authenticated only) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the api requires authentication but does not require the permission.
I would go with public api - without authentication and permission. It can be useful for calculator page for example.
or If we want it to be authenticated, then we should add a permission for it.
Since the response may has info about discounts, I would go with authenticated and permission checked api.
fc3b846 to
3e6cc27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the PR doesn't include updates for the generated code.
Please make sure you've run the following commands from the root directory:
make generateOnce you've added them, you can dismiss this review.
lomidzemikheili
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Thanks for addressing the feedback
Introducing a new endpoint called
quote. Users with thewrite:clusterpermission will be able to get the pricing for the cluster included any available discounts.