Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 testing fixtures and a basic test #52
Feat : add testing fixtures and a basic test #52
Changes from 8 commits
726f1ed
b3a80b0
5826d44
0e70fff
97f1f21
a64613c
db472d5
15541ef
fc7d493
9a101ba
906a1eb
65d66e6
20d82c3
63bc846
f36b483
8b7e68f
74fea52
385974d
c01c990
b31212c
ae2d4d9
67141ba
6c9b68e
20a480e
7f01895
44aecd7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
Why is this only used in tests? Is there no equivalent notion when connecting to AWS directly? It would be better to avoid this difference between production and testing, if possible.
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.
Localstack endpoint URL : http://localhost.localstack.cloud:4566
AWS endpoint URL. : https://s3.{region}.amazonaws.com/{bucket-name}/{key}
We are using localstack for mocking AWS locally.
Localstack requires
endpoint_url
to get connected.Connection to AWS doesn't necessarily require the endpoint url as shown here.
Rather, the connection to localstack does as shown here.
Hence this implementation is under the tests flag.
What do you propose as a solution?,
Do you think changing prod code as how tests are written (adding endpoint url on prod just because it's available on tests) is a valid way to go ?
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.
But specifying the endpoint is still a possibility on AWS, right? I'm not in favor of "modifying production code", but here it's just that you have 2 different ways of connecting: either you specify the region, bucket key etc or you provide the endpoint directly. It turns out that localstack only supports the endpoint method, sure.
In effect you have two usable connection solutions => you could use an enumeration to support both features, for example giving priority to the endpoint method if someone wants to hardcode it for some reason and then support the default method as a backup. This way, your code is the same for testing vs prod and you support two methods to connect to AWS. WDYT?
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 @odesenfans
I agree with your implementation and it is more cleaner. I will implement this and push the code.
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.
Nitpick: when I see the call to
clone()
, my first reflex is to check the cost of this operation. I looked at the docs of MongoDB and saw thatClient
uses anArc
internally, so this is fine. But it is worth adding a comment explaining why this is fine, because for non Arc-based types doingand then letting the caller decide if he wants to clone is better.
TL;DR, add a comment to explain that
Client
usesArc
and therefore is cheap to clone.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.
Valid and a great catch,
added docs to describe that
client
usesArc
.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.
How is this change related to the PR?
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.
This is a valid concern,
While the tests for
Job_controller
were being written it was realised that we are not using it anymore and hence it can be deleted, we agree, ideally this is to be done on a separate PR.Proposed Solution :
job_controller
in theCHANGELOG
.WDYT ?
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.
You can keep it in this PR, but mention it in the changelog and in the PR description.
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.
sure
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.
Careful that you'll need to add every future collection in here as well. It might be better to just drop every collection under the
orchestrator
DB.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.
Valid, changed to dropping all the collections for now.