Skip to content

fix: change syncTableTags parameter order to avoid panic #136

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

Merged
merged 1 commit into from
May 19, 2025

Conversation

michaelhtm
Copy link
Member

Issue #2459#issuecomment-2886623397

Description of changes:
In syncTableTags, there was an issue with the order/naming of desired vs
latest resource.

Although the function is called using syncTableTags(desired, latest)
the function definition had syncTableTags(latest, desired). This
becomes an issue when we try to retrieve the ARN from latest, during
adoption.
Although in the funciton we named the parameter latest, this parameter
was treated as the desired resource when computing the tags delta.

During adoption, the Status/ARN of the desired resource is meant to be
nil, unitl it is later populated. Hence our need to use the ARN from
latest.

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

In syncTableTags, there was an issue with the order/naming of desired vs
latest resource.

Although the function is called using `syncTableTags(desired, latest)`
the function definition had `syncTableTags(latest, desired)`. This
becomes an issue when we try to retrieve the ARN from latest, during
adoption.
Although in the funciton we named the parameter `latest`, this parameter
was treated as the desired resource when computing the tags delta.

During adoption, the Status/ARN of the desired resource is meant to be
nil, unitl it is later populated. Hence our need to use the ARN from
latest
@ack-prow ack-prow bot requested review from knottnt and rushmash91 May 16, 2025 16:55
@ack-prow ack-prow bot added the approved label May 16, 2025
Copy link

ack-prow bot commented May 16, 2025

@michaelhtm: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
dynamodb-verify-attribution 23b9039 link false /test dynamodb-verify-attribution

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@knottnt
Copy link

knottnt commented May 19, 2025

/lgtm

@ack-prow ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label May 19, 2025
Copy link

ack-prow bot commented May 19, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knottnt, michaelhtm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-prow ack-prow bot merged commit 13b019e into aws-controllers-k8s:main May 19, 2025
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants