-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: Add AWS SQS Sink functionality #1
Conversation
Signed-off-by: chandankumar4 <chandan.kr404@gmail.com>
Signed-off-by: chandankumar4 <chandan.kr404@gmail.com>
Signed-off-by: chandankumar4 <chandan.kr404@gmail.com>
10b3441
to
ca8dcb6
Compare
.github/workflows/ci.yaml
Outdated
- name: "Step 6: Load image in cluster" | ||
run: | | ||
docker load --input /tmp/aws-sqs-queue.tar | ||
kind load docker-image "quay.io/numaio/numaproj-contrib/aws-sqs-sink-go:sqs-e2e" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this working? kind
?
main.go
Outdated
if !ok { | ||
logger.Fatalln("AWS_REGION not found") | ||
} | ||
accessKey, ok := os.LookupEnv("AWS_ACCESS_KEY") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The credentials should not be mandatory, ppl might use things like IAM Role for service account to access.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add unit tests.
main.go
Outdated
queueName string | ||
region string | ||
awsAccessKey string | ||
awsAccessSecret string | ||
awsBaseEndpoint string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider using a config object?
main.go
Outdated
if !ok { | ||
logger.Fatalln("AWS_REGION not found") | ||
} | ||
accessKey, ok := os.LookupEnv("AWS_ACCESS_KEY") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
main.go
Outdated
if _, err = s.sqsClient.SendMessage(ctx, &sqs.SendMessageInput{ | ||
MessageBody: &msgBody, | ||
QueueUrl: queueURL.QueueUrl, | ||
}); err != nil { | ||
s.logger.Errorf("failed to push message %v", err) | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invoking send for each message might be expensive.. Consider writing in batches?
Signed-off-by: chandankumar4 <chandan.kr404@gmail.com>
Signed-off-by: chandankumar4 <chandan.kr404@gmail.com>
Signed-off-by: chandankumar4 <chandan.kr404@gmail.com>
limitations under the License. | ||
*/ | ||
|
||
package fixtures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should extract one out for contrib org.
Signed-off-by: chandankumar4 <chandan.kr404@gmail.com>
Signed-off-by: chandankumar4 <chandan.kr404@gmail.com>
Signed-off-by: chandankumar4 <chandan.kr404@gmail.com>
Sry one more comment, are we suppose to see CI workflow succeeds? I don't see checks running for this PR. |
yaa, I think it will trigger after first PR merge to main branch with workflow yaml but not sure (that's what I have observed in previous repo) maybe someone need to enable it manually? |
Also unit test case is pending, I'm working on that. probably we merge it also and I'll raise another PR for it? |
Sounds good. I am approving this PR. Feel free to merge it and use another PR to trigger CI. |
I don't have access to merge ;) |
Done. |
Add the functionality to sink pipeline vertex data to aws queue. Please check README for example pipeline
Fixes: numaproj/numaflow#1093