Conversation
trstephen-amazon
left a comment
trstephen-amazon
left a comment
There was a problem hiding this comment.
Overall looks fine but at minimum the license comment needs to be retained.
| gem 'aws-sigv4', '1.10.1' | ||
|
|
||
| gem "aws-sdk-dsql", "~> 1" | ||
| gem 'aws-sdk-core', '3.216.0' |
There was a problem hiding this comment.
nit: don't think this is needed, was just for hand-rolled sigv4 token generation
| ruby --version | ||
| ``` | ||
|
|
||
| For example: `v3.2.6"`. |
There was a problem hiding this comment.
nit: CI is using 2.5 which has been EOL since 2017. Can update in a separate PR.
There was a problem hiding this comment.
Yes, initially in line 53 we used to have
Ensure you have ruby v2.5+ installed
We changed it to
.. have ruby v3.2+ installed
Should we revert line 53 to v2.5?
Perhaps the line 61 stating
For example:
v3.2.6"
Is not even needed?
| ### Set the environmet variables specifying cluster endpoint, region and cluster user | ||
|
|
||
| ``` | ||
| # e.g. 'admin' or 'non_admin_user' |
There was a problem hiding this comment.
nit: implies two valid values. Can leave this open ended with something like "e.g. 'admin' or a custom user"
|
|
||
| rspec | ||
| # e.g. "us-east-1" | ||
| export REGION="<your cluster region>" |
There was a problem hiding this comment.
nit: other examples call this CLUSTER_REGION
There was a problem hiding this comment.
Agree that it would probably be better and consistent with the other related variable names. However, most languages at the moment call it REGION in code samples and readme.md.
Exceptions:
Java, typescript and secret variable names in GitHub action yml files.
I guess we should standardize? Make all to use CLUSTER_REGION?
| Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| Execute the following command: | ||
|
|
||
| SPDX-License-Identifier: MIT-0 |
There was a problem hiding this comment.
License ID should be retained
| password_token = token_generator.generate_db_connect_auth_token({ | ||
| :endpoint => cluster_endpoint, | ||
| :region => region | ||
| }) |
There was a problem hiding this comment.
nit: file mixes hash styles. PG.connect uses implicit hashes (no {}) and map syntax but token methods above use explicit hashes and 'hash rockets'. See this file for a token generator with the PG.connect style syntax.
|
|
||
| Before using the Ruby-pg driver, ensure you have the following prerequisites installed: | ||
| Ruby: Ensure you have ruby v3.2+ installed from the [official website](https://www.ruby-lang.org/en/documentation/installation/). | ||
| Ruby: Ensure you have ruby v2.5+ installed from the [official website](https://www.ruby-lang.org/en/documentation/installation/). |
There was a problem hiding this comment.
Disagree here since the version has been EOL for over 7 years. If something breaks because our ruby version is too old we're unlikely to backport a fix and just drop compatibility. Might as well short-circuit that now and bump the supported version. Can you update the CI to use ruby 3+?
| @@ -51,6 +51,8 @@ jobs: | |||
| env: | |||
There was a problem hiding this comment.
bah, L37 and L40 are out of scope for "suggest changes" functionality.
anyway! Can you change those 2.5 -> 3.3 so CI builds against our advertised supported version?
| @@ -51,6 +51,8 @@ jobs: | |||
| env: | |||
4 participants
This PR updates the Ruby-pg example.
Description of changes:
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT-0 license.