Skip to content

Conversation

omkhegde
Copy link
Contributor

Issue #, if available: #292

Description of changes: This change adds getPreviousResourceTags method which returns previous tags that were applied to a resource. The tags come from previousStackTags and previousResourceDefinedTags

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

    Description of changes: This change adds getPreviousResourceTags method which returns previous tags that were applied to a resource.
    The tags come from previousStackTags and previousResourceDefinedTags

    By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@wbingli
Copy link

wbingli commented Aug 20, 2020

LGTM. Let's verify this change with one of the resources e.g. R53 HostedZone. Once we confirm it works (Update handler can get the previousResourceTags), will approve it.


if (request != null && request.getRequestData() != null) {
replaceInMap(previousResourceTags, request.getRequestData().getPreviousStackTags());
replaceInMap(previousResourceTags,
Copy link

Choose a reason for hiding this comment

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

The getPreviousResourceProperties() is null for CREATE/DELETE/READ, and not all provideResourceDefinedTags handles the null resource model. let's have a null check here.

if (request.getRequestData().getPreviousResourceProperties() != null) {
 replaceInMap(previousResourceTags,provideResourceDefinedTags(request.getRequestData().getPreviousResourceProperties()));
}

Copy link
Member

@xiwhuang xiwhuang left a comment

Choose a reason for hiding this comment

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

Tested with HostedZone resource.

Copy link

@wbingli wbingli left a comment

Choose a reason for hiding this comment

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

Backward compatibility concern change.

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