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

capabilities: reconciliate product on backend metric deletion #444

Merged
merged 2 commits into from
Sep 15, 2020

Conversation

eguzki
Copy link
Member

@eguzki eguzki commented Jul 31, 2020

When backend metric or method is deleted, Application plan limits and pricing rules with references to it have to be deleted also, as it happens automatically in the System UI.

Fixes issue about product and backend consistency when there are references between them.

@eguzki eguzki force-pushed the capabilities-backend-consistency branch 2 times, most recently from da77b87 to 81f5c11 Compare August 26, 2020 15:26
@eguzki eguzki force-pushed the capabilities-backend-consistency branch 4 times, most recently from b4a335a to 40be156 Compare September 1, 2020 14:43
@eguzki eguzki force-pushed the capabilities-backend-consistency branch from 40be156 to 0a62aef Compare September 4, 2020 14:58
@eguzki eguzki force-pushed the capabilities-backend-consistency branch from 0a62aef to 9aafa60 Compare September 14, 2020 16:39
@eguzki
Copy link
Member Author

eguzki commented Sep 14, 2020

ping @miguelsorianod

@miguelsorianod
Copy link
Contributor

I was waiting for your input here.

After we talked about this you were going to review this approach (let it fail vs try to fix it, etc...). Have you reviewed it?

@eguzki
Copy link
Member Author

eguzki commented Sep 15, 2020

You are the one in the asignees section :) This is ready to be reviewed.

@miguelsorianod
Copy link
Contributor

? That does not change what was talked and the action points of the other day. I was expecting you to review it and give me feedback based on that.
If it helps you I've re-assigned it to you. Let me know your views on that and why one decision has been taken in front of the other

@eguzki
Copy link
Member Author

eguzki commented Sep 15, 2020

This PR implements a "Product CR Spec" update from the Backend Controller to make it valid from a invalid state. This adds robustness to the operator, kind of healing capability when some spec becomes invalid due to an external action. This same behavior has been seen by 3scale internal System. When backend metric or method is deleted in the dashboard, the application plan limits and pricing rules with references to it are automatically deleted also.

The other approach is, do not react and do not "heal" invalid CR. If backend metric is deleted, the product spec will be invalid, as existing backend reference no longer exists. The operator would report as events, CR status and internal log and manual action would be required.

Both approaches can be valid. Since 3scale UI "heals" application plans limits and pricing rules automatically, for consistency shake, I would add this feature to the operator.

@eguzki eguzki assigned miguelsorianod and unassigned eguzki Sep 15, 2020
Copy link
Contributor

@miguelsorianod miguelsorianod left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed information and insight @eguzki 👍

I'm fine with this approach. Both approaches seem acceptable solutions to me with some pros and cons for each one.

I left a minor comment. All other changes look good to me 👍

pkg/controller/helper/product_list.go Outdated Show resolved Hide resolved
@eguzki eguzki force-pushed the capabilities-backend-consistency branch from 212bf7a to 1f81d05 Compare September 15, 2020 16:33
@codeclimate
Copy link

codeclimate bot commented Sep 15, 2020

Code Climate has analyzed commit 1f81d05 and detected 9 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 2
Duplication 6
Style 1

View more on Code Climate.

@eguzki eguzki assigned miguelsorianod and unassigned eguzki Sep 15, 2020
Copy link
Contributor

@miguelsorianod miguelsorianod left a comment

Choose a reason for hiding this comment

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

👍

@eguzki eguzki merged commit ec9187a into master Sep 15, 2020
@eguzki eguzki deleted the capabilities-backend-consistency branch September 15, 2020 16:51
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.

2 participants