-
Notifications
You must be signed in to change notification settings - Fork 5
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
Switch AWS clients over to a generic cloud client library so it can run on other locations. #8
Conversation
I haven't actually tried this out yet, and probably won't be able to on AWS since I don't have an AWS account. I'll get it running on GCP today though. |
OK, this is working now for GCP. The instructions to setup need to change a bit, the main difference is the environment variables used to access the queue/storage. They basically follow the patterns from here: https://github.com/google/go-cloud For GCP, you can use something like: OSSMALWARE_UPLOAD_BUCKET_URL=gs://<gcs bucket name>
OSSMALWARE_QUEUE_URL=gcppubsub://projects/<gcp project id>/subscriptions/<gcp pubsub subscription> For AWS, it would look like: OSSMALWARE_UPLOAD_BUCKET_URL=s3://<s3 bucket name>
OSSMALWARE_QUEUE_URL=awssqs://sqs.us-east-2.amazonaws.com/123456789012/myqueue |
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.
Hey @dlorenc! This is looking great.
I'll give this a shot on AWS, but to be honest I think it's fine moving forward with GCP specifically, since we'll eventually have a series of cloud functions that fetch packages from different package managers, and at that point it'll be trickier to be multi-cloud.
pkg/consumer/consumer.go
Outdated
} | ||
return consumer, nil | ||
return &PubSubConsumer{ | ||
url: queueURL, |
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.
Out of curiosity - do we still need the url
field? It seems that Next()
now just needs the subs
attribute.
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.
Nope, it's only required for the logging line. I can't find a way to get the URL back out of the subs
object, so I saved the URL. Let me know if you'd prefer to drop that logging line or switch it to something else.
Also, one more thing: we'll need to update the log line here since it previously pointed to the bucket URL and now points to the bucket object:
Otherwise, this seems to work fine on AWS, too, which is great! |
Ready for another look! |
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.
Hey @dlorenc! This looks great. There was a super minor typo that slipped in, but when that's resolved we'll be in great shape to merge.
Thanks for all the great work!
All set! |
No description provided.