Conversation
anwesham-lab
left a comment
There was a problem hiding this comment.
We need to update the tests with the correct paths now, ie examples_preferred. I think we should have a smoke test for all examples being used so you'll want the tests repo to be refactored
We'll also want to update the driver-layer README.md files to keep the default running example_preferred.py.
You can add the follow-up explaining the alternatives and why they're present in a separate PR if it makes more sense.
This sample will give you some examples of how to refactor the tests in the python case for example: #215
f892447 to
b041480
Compare
f7ee006 to
a51fa04
Compare
61321bd to
f743b80
Compare
|
It's a very long PR. Here is a summary of what I moved around and what is new: SummaryRestructures Python, JavaScript, and Java examples to follow a consistent folder structure with GoalMake StructureChangesCopied from connector repos:
New code:
Restored from pre-#221 / pre-#198:
Notes
Java restructure:
Other:
Related PRs
Next Steps
|
| "type": "module", | ||
| "scripts": { | ||
| "test": "node --experimental-vm-modules node_modules/.bin/jest --testPathPattern='test/smoke.test.js' --runInBand --detectOpenHandles --forceExit" | ||
| "test": "node --experimental-vm-modules node_modules/.bin/jest --testPathPattern='test/example_preferred.test.js' --runInBand --detectOpenHandles --forceExit" |
There was a problem hiding this comment.
Does this imply we're only running the one test? We may want to remove the testPathPattern flag entirely like what was done for the Node.js connectors change. In the other PR the integration tests run all of the examples which is better imo.
There was a problem hiding this comment.
Missed that, will fix for this PR.
| "type": "module", | ||
| "scripts": { | ||
| "test": "node --experimental-vm-modules node_modules/.bin/jest --testPathPattern='test/smoke.test.js' --runInBand --detectOpenHandles --forceExit" | ||
| "test": "node --experimental-vm-modules node_modules/.bin/jest --testPathPattern='test/example_preferred.test.js' --runInBand --detectOpenHandles --forceExit" |
There was a problem hiding this comment.
Same comment here, I think we want to run all of the tests rather than just the one.
There was a problem hiding this comment.
Thanks, fair comment, will fix that.
| echo "$GITHUB_WORKSPACE" >> $GITHUB_PATH | ||
| wget https://www.amazontrust.com/repository/AmazonRootCA1.pem -O root.pem | ||
| python src/example.py | ||
| python src/example_preferred.py |
There was a problem hiding this comment.
It seems like we should invoke pytest for both of the Psycopg workflows rather than the example script directly. Currently only the one example is exercised, and the tests aren't used at all.
There was a problem hiding this comment.
Thanks, will add testing for all of them.
| raise FileNotFoundError(f"SSL certificate file not found: {ssl_cert_path}") | ||
|
|
||
| conn_params = { | ||
| "dbname": "postgres", |
There was a problem hiding this comment.
Do we want to include things that have sensible defaults in the connector, like dbname, port sslmode? I'd have to test to make sure but I think these can be safely removed here as we've done elsewhere.
There was a problem hiding this comment.
I agree with you. I think this should be done in follow-up. The connectors are the source of truth, so we will need to update the code there. Currently the code here is duplicate/same as what we have in the connectors.
The next steps would look something like:
- Improve connector code to remove the sensible defaults. Update the readmes to include info about the defaults
- Automate
.. now dsql-samples will have that.
Let me know if that sounds reasonable.
There was a problem hiding this comment.
Yep that sounds good to me
| raise FileNotFoundError(f"SSL certificate file not found: {ssl_cert_path}") | ||
|
|
||
| conn_params = { | ||
| "dbname": "postgres", |
There was a problem hiding this comment.
Same comment about using DSQL defaults to simplify UX for "preferred" sample
There was a problem hiding this comment.
Same comment as above
| "user": cluster_user, | ||
| "host": cluster_endpoint, | ||
| "port": "5432", | ||
| "region": region, |
There was a problem hiding this comment.
I'm not sure the region is needed here given the endpoint contains the region for the connectors.
There was a problem hiding this comment.
Same comment as above
| "user": cluster_user, | ||
| "host": cluster_endpoint, | ||
| "port": "5432", | ||
| "region": region, |
There was a problem hiding this comment.
Same comment, not sure region is needed.
There was a problem hiding this comment.
Same comment as above
Restructures Python, JavaScript, and Java examples with example_preferred as the golden path (pool + connector). - Python/JS: Copied pool examples from connector repos, restored SDK-only from pre-#221 - Java: Merged pgjdbc_hikaricp into pgjdbc, restored SDK-only from pre-#198 - Added README in each alternatives/ folder - Added smoke tests for all examples
f743b80 to
e06f582
Compare
Summary of changes in commit message:
Restructures Python, JavaScript, and Java examples with
example_preferred as the golden path (pool + connector).
restored SDK-only from pre-Update samples to use DSQL connectors #221
restored SDK-only from pre-java: Use JDBC connector in pgJDBC sample #198
Notes
Refer to: https://amzn-aws.slack.com/archives/C09UL3KG458/p1766193841285889
Following: