fix: deprecate tag logic on Association #2552
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
src/sagemaker/lineage/association.py
Outdated
| @@ -64,28 +64,6 @@ def delete(self): | |||
| """Delete this Association from SageMaker.""" | |||
| self._invoke_api(self._boto_delete_method, self._boto_delete_members) | |||
|
|
|||
| def set_tag(self, tag=None): | |||
| """Add a tag to the object. | |||
There was a problem hiding this comment.
associations don't have arns so this tagging logic doesn't make sense
Hey @danabens, thanks for the contribution! Looking at the description for this class, an association object still has a source and destination arn as attributes. Has this behavior changed?
Also, can we deprecate this instead and communicate this change instead of deleting it first, and then delete it eventually ? I am concerned with downstream consumers of the lineage APIs, who might be currently setting tags as part of their workflows and might find those workflows failing when they pull in the release with this change. Thanks so much!
There was a problem hiding this comment.
Has this behavior changed?
No, association does have a source and dest but does not have an arn itself so it never made sense to have tagging behavior on this class in the first place. Consumers can tag the source and dest in other classes or by calling add_tags directly.
can we deprecate this instead
Ya, good idea.
There was a problem hiding this comment.
Thanks for the clarification! Waiting for tests to pass before merge
danabens
force-pushed
the
remove-set-tag
branch
from
b77eb48 to
b8ee989
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
danabens
changed the title
danabens
force-pushed
the
remove-set-tag
branch
from
b8ee989 to
f246eb2
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
associations don't have arns so this tagging logic doesn't make sense
Merge Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
unique_name_from_baseto create resource names in integ tests (if appropriate)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.