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

S3 to S3 Replication in SAM #2062

Merged

Conversation

jorgetovar
Copy link
Contributor

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@bfreiberg
Copy link
Contributor

Hi @jorgetovar , thanks for the pull request. However, your submission does not represent a serverless pattern as we see it. Those are typically intended to be primarily IaC-focused implementations of 2-4 AWS services. Feel free to expand upon this and resubmit then.

@bfreiberg bfreiberg closed this Jan 24, 2024
@jorgetovar
Copy link
Contributor Author

But I see a similar pattern here but with CDK

https://serverlessland.com/patterns/s3-s3-cdk

@bfreiberg
Copy link
Contributor

Thanks for your feedback. We've discussed it and will accept this pattern, also given the pattern you referenced. I'll review your submission soon.

@bfreiberg bfreiberg reopened this Jan 31, 2024
s3-s3-replication-sam/README.md Outdated Show resolved Hide resolved
s3-s3-replication-sam/README.md Show resolved Hide resolved
s3-s3-replication-sam/README.md Outdated Show resolved Hide resolved
s3-s3-replication-sam/README.md Outdated Show resolved Hide resolved
s3-s3-replication-sam/README.md Outdated Show resolved Hide resolved
s3-s3-replication-sam/example-pattern.json Outdated Show resolved Hide resolved
s3-s3-replication-sam/example-pattern.json Outdated Show resolved Hide resolved
s3-s3-replication-sam/template.yaml Outdated Show resolved Hide resolved
s3-s3-replication-sam/template.yaml Outdated Show resolved Hide resolved
s3-s3-replication-sam/template.yaml Outdated Show resolved Hide resolved
@bfreiberg
Copy link
Contributor

@jorgetovar , do you still plan to work on this issue?

Copy link
Contributor Author

@jorgetovar jorgetovar left a comment

Choose a reason for hiding this comment

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

Thanks, @bfreiberg, for all the suggestions. They have been addressed in the pull request now.

s3-s3-replication-sam/example-pattern.json Show resolved Hide resolved
@jorgetovar
Copy link
Contributor Author

@bfreiberg

Please let me know if there are any specific areas you'd like me to address or if you need any further information from my end.

Thank you for your time and good advice

@bfreiberg
Copy link
Contributor

Looks good in general. I've highlighted a few minor adjustments

@jorgetovar
Copy link
Contributor Author

@bfreiberg

Please let me know if there are anything else to address or if you need any further information from my end.

@jorgetovar
Copy link
Contributor Author

jorgetovar commented Apr 4, 2024

Looks good in general. I've highlighted a few minor adjustments

Looking forward to see if it's anything else missing. And thanks for all the assistance

@bfreiberg
Copy link
Contributor

Looks pretty good, thanks for the updates. I highlighted two minor changes. It would be great if you could address them

@jorgetovar
Copy link
Contributor Author

I will work on that today

@jorgetovar
Copy link
Contributor Author

I just had my son, so I was a bit delayed. Link updated. Everything should be ready for the merge now. Thank you for the assistance

@bfreiberg

@jorgetovar
Copy link
Contributor Author

Hello looking forward for any updates:)

s3-s3-replication-sam/README.md Outdated Show resolved Hide resolved
s3-s3-replication-sam/example-pattern.json Outdated Show resolved Hide resolved
@bfreiberg
Copy link
Contributor

I'm really sorry but I missed the incorrect service name before. Once that is changed, the PR is good to go

@bfreiberg
Copy link
Contributor

Hi @jorgetovar , the service name still hasn't been changed. Are you sure that you pushed your latest changes?

@jorgetovar
Copy link
Contributor Author

That's strange. I have committed the suggestions and double-checked them, and they should be correct now. Sorry about that.

@bfreiberg
Copy link
Contributor

Great, thanks @jorgetovar for your contribution. A developer advocate will merge your PR soon

@julianwood julianwood merged commit 2f4a0e3 into aws-samples:main Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants