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
Fix SQS CFn updates with no change #9831
Conversation
f6d395f
to
5dfb519
Compare
5dfb519
to
5c8f143
Compare
should_replace = ( | ||
request.desired_state.get("QueueName", request.previous_state["QueueName"]) | ||
!= request.previous_state["QueueName"] | ||
) or ( | ||
request.desired_state.get("FifoQueue", request.previous_state["FifoQueue"]) | ||
!= request.previous_state["FifoQueue"] | ||
) |
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.
@dominikschubert there must be a way to automate this sort of detection from the spec... 🤔
# this is an issue with our update detection. We should never be in this state. | ||
request.action = "Add" |
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.
so is this a TODO for the framework? or what do we do about the 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.
This is an issue with the CFn engine in LocalStack. We should never try to perform an update/modify on a resource that has no previous state, since this means the resource has not been created yet.
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 think it's fine as a fix for now until the update detection is fixed
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! thanks for adding the test
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! thanks for making this cover more use cases and upgrading my very naive fix!
Motivation
If a CFn stack containing an SQS queue is updated, but the SQS queue is not changed, we trigger invalid behaviour where we assume information in the incoming request that is not present. Specifically, we try to get the queue name from the incoming state, and if this was not specified in the first place (i.e. we rely on auto-generating the queue name) the stack fails to update.
While investigating this issue I found another issue when adding a completely new resource. The "update" method is still invoked, but because we raise a
NotImplementedError
we try to handle this in the executor, but this assumes there is a previous state. For a completely new resource, this is not the case.Closes #9826
Changes