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

Add actionability parameter to the rule baseclass. #243

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Max-Huneshagen
Copy link
Contributor

We add an actionability parameter to the base class rule. If True, this parameter indicates that the non-fulfilment of the rule can be solved without recreating any resource and/or stack. The standard value is set to True for all rules.

@Max-Huneshagen Max-Huneshagen marked this pull request as ready for review November 3, 2023 16:10
@Max-Huneshagen Max-Huneshagen requested a review from a team November 3, 2023 16:10
Copy link
Member

@w0rmr1d3r w0rmr1d3r left a comment

Choose a reason for hiding this comment

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

The field property for this new value is missing

@@ -15,6 +15,7 @@


class Rule(ABC):
ACTIONABILITY = True
Copy link
Member

Choose a reason for hiding this comment

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

I'd rename the field to: ACTIONABLE instead of ACTIONABILITY
Since it'll be either actionable or non-actionable. Actionability sounds "weird"

Suggested change
ACTIONABILITY = True
ACTIONABLE = True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we also want to include the new property in the config or not?

Copy link
Member

Choose a reason for hiding this comment

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

I wasn't sure of it when I added my other comment. Now that I think of it, we can safe proof it, and add it to the config with the same value. This way, if some user/dependency of cfripper needs to override the value, it can be done safely in their config 👍

@jsoucheiron
Copy link
Member

jsoucheiron commented Nov 6, 2023

Maybe it'd be worth to add something a bit more extensible. We could add a variable called SOLUTION_TYPE it could be an enum with a few values: REQUIRES_CF_TEARDOWN, REQUIRES_RESOURCE_RECREATION, IN_PLACE_UPDATE...

@ignaciobolonio ignaciobolonio marked this pull request as draft December 28, 2023 10:47
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.

3 participants