Skip to content
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 test for changed body when putting tag #71

Closed
wants to merge 1 commit into from

Conversation

elgohr
Copy link
Contributor

@elgohr elgohr commented Nov 25, 2022

At the moment the body is changed when putting a tag.
I just created the test . Could you please have a look at the implementation?

@johannesboyne
Copy link
Owner

Thanks a lot for the test-case! As it currently fails (obviously), it would be best to also include the implementation! I have not looked at the implementation details behind the tagging, but it looks straight forward here https://docs.aws.amazon.com/AmazonS3/latest/API/API_PutObjectTagging.html

PUT /{Key+}?tagging&versionId=VersionId HTTP/1.1 
Host: Bucket.s3.amazonaws.com 
Content-MD5: ContentMD5 
x-amz-sdk-checksum-algorithm: ChecksumAlgorithm 
x-amz-expected-bucket-owner: ExpectedBucketOwner 
x-amz-request-payer: RequestPayer 
<?xml version="1.0" encoding="UTF-8"?>
  <Tagging xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
  <TagSet>
    <Tag>
      <Key>string</Key>
      <Value>string</Value>
    </Tag>
  </TagSet>
</Tagging>

The implementation would be probably as follows:

  1. Adding a route https://github.com/johannesboyne/gofakes3/blob/master/routing.go#L21
  2. Followed by a new function within gofakes3.go that implements the tagging correctly

Are you open to that?

Just adding the failing test-case could cause other implementations to fail if they check libs for test-cases.

@elgohr
Copy link
Contributor Author

elgohr commented Nov 25, 2022

So how would you put that into https://github.com/johannesboyne/gofakes3/blob/master/backend.go#L133 ?
There're a few concepts, like the homebrewn router, that take me time to understand.
I guess it would be faster when you would give it a try.

@johannesboyne
Copy link
Owner

Worked on in PR #75

@elgohr
Copy link
Contributor Author

elgohr commented Dec 29, 2022

Would have been great if you cherry-picked my commit - but I'm happy anyway for the topic to move on

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.

None yet

2 participants