-
Notifications
You must be signed in to change notification settings - Fork 86
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 delete flag to external-resources #4521
Conversation
resource.dict( | ||
exclude={"data": {FLAG_RESOURCE_MANAGED_BY_ERV2, FLAG_DELETE_RESOURCE}} | ||
) |
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.
why do we need to exclude those fields? There is a similar method serialize_input
in ExternalResource
, and to serialize json, can directly call resource.json(
instead of resource.dict(
then json.dumps
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.
-
These fields are not expected in the module input. Since the
overrides
parameter doesn't have a schema, all the overridden parameters are passed to the terraform object directly (the same way as terraform-resources). So all parameters not expected in the input shall be removed. -
I changed the call to
resource.json()
and removed the method in ExternalResource (this was used in an old implementation when the INPUT was passed as an env var)
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.
Is there a way to ignore unexpected input or define a clear mapping? Otherwise we have to remember edit this part whenever we put new fields in data
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 cannot think of anything to do that as we want the ability to add additional fields in the overrides.
There should not be a lot of changes here since only "logical" parameters need to be introduced.
I agree though, I'll try to overcome this or at least have a way to detect it with a test or something.
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.
LGTM!
External resources deletion will work with the
delete: true
attribute at the resource level.Why:
deletion_approvals
are meant to avoid non-wanted resource deletions in case resource specs are removed from app-interface. Requiring adelete: true
attribute accomplishes the same objective.Schema update: app-sre/qontract-schemas#683