Closed randytpierce closed 4 months ago
.env files? I didn't even see those, thanks.
On Fri, Mar 1, 2024 at 11:16 AM Ian McGinnis @.***> wrote:
@.**** requested changes on this pull request.
Generally looks good. Though the .env files should be removed & .gitignore file updated.
Otherwise, I was curious if we need to add some validation that the cb_credentials["host"] variable has the couchbase URI prefix.
On .env-capella https://github.com/NOAA-GSL/VxIngest/pull/341#discussion_r1509368270:
This shouldn't be checked in since it's user-specific. Maybe we should update the .gitignore to exclude .env* patterns?
On .env-cb1 https://github.com/NOAA-GSL/VxIngest/pull/341#discussion_r1509368416:
Same comment here as the .env-capella file.
In src/vxingest/builder_common/ingest_manager.py https://github.com/NOAA-GSL/VxIngest/pull/341#discussion_r1509370768:
- self.cluster = Cluster(
- "couchbase://" + self.cb_credentials["host"], options
- )
- self.cluster = Cluster(self.cb_credentials["host"], options)
Do we need to do any validation to make sure the couchbase:// or couchbases:// strings are in the cb_credentials["host"] field?
— Reply to this email directly, view it on GitHub https://github.com/NOAA-GSL/VxIngest/pull/341#pullrequestreview-1911749357, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGDVQPTRPIZV37OIEUYUAG3YWDAZHAVCNFSM6AAAAABECE4O5KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMJRG42DSMZVG4 . You are receiving this because you were assigned.Message ID: @.***>
-- Randy Pierce
I removed and ignored the .env files. I think the tests do check the protocol and so does the code. I'm not sure we should add specific validation for the host. If the host is wrong the connection error messages are pretty obvious and what if the protocol changes? randy
On Fri, Mar 1, 2024 at 11:16 AM Ian McGinnis @.***> wrote:
@.**** requested changes on this pull request.
Generally looks good. Though the .env files should be removed & .gitignore file updated.
Otherwise, I was curious if we need to add some validation that the cb_credentials["host"] variable has the couchbase URI prefix.
On .env-capella https://github.com/NOAA-GSL/VxIngest/pull/341#discussion_r1509368270:
This shouldn't be checked in since it's user-specific. Maybe we should update the .gitignore to exclude .env* patterns?
On .env-cb1 https://github.com/NOAA-GSL/VxIngest/pull/341#discussion_r1509368416:
Same comment here as the .env-capella file.
In src/vxingest/builder_common/ingest_manager.py https://github.com/NOAA-GSL/VxIngest/pull/341#discussion_r1509370768:
- self.cluster = Cluster(
- "couchbase://" + self.cb_credentials["host"], options
- )
- self.cluster = Cluster(self.cb_credentials["host"], options)
Do we need to do any validation to make sure the couchbase:// or couchbases:// strings are in the cb_credentials["host"] field?
— Reply to this email directly, view it on GitHub https://github.com/NOAA-GSL/VxIngest/pull/341#pullrequestreview-1911749357, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGDVQPTRPIZV37OIEUYUAG3YWDAZHAVCNFSM6AAAAABECE4O5KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMJRG42DSMZVG4 . You are receiving this because you were assigned.Message ID: @.***>
-- Randy Pierce
Actually... either way - it'd be good to update the README
with a correct example for the $HOME/credentials
file. The current one is now misleading. https://github.com/NOAA-GSL/VxIngest?tab=readme-ov-file#usage
Sure, The connection failure isn't really that difficult to understand and I think we may end up needing https or even http for specific cases like a laptop couchbase install. randy
On Fri, Mar 1, 2024 at 11:43 AM Ian McGinnis @.***> wrote:
@.**** approved this pull request.
Thanks for removing the .env's!
I agree that it'd be inappropriate to check for a specific hostname. I was thinking it may be helpful to check for the presence of the couchbase:// or couchbases:// prefix in the host field in the config. My understanding is that the host field now requires those prefixes where it didn't before. E.g.
An old config like:
host: "some.host.com"
Now needs to be:
host: "couchbase://some.host.com"
Or:
host: "couchbases://some.host.com"
I could see connection failures being tricky to understand otherwise if the Couchbase SDK doesn't point out that you needed the protocol in addition to the hostname. I don't think this is a requirement for this PR though so feel free to merge and we can put it in another issue if we decide it's necessary. Mostly it just seemed like a nice way to get a better error message.
— Reply to this email directly, view it on GitHub https://github.com/NOAA-GSL/VxIngest/pull/341#pullrequestreview-1911806716, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGDVQPUW2LJEJBS6TUDCA53YWDD55AVCNFSM6AAAAABECE4O5KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMJRHAYDMNZRGY . You are receiving this because you were assigned.Message ID: @.***>
-- Randy Pierce
Sure. Done. randy
On Fri, Mar 1, 2024 at 11:44 AM Ian McGinnis @.***> wrote:
Actually... either way - it'd be good to update the README with a correct example for the $HOME/credentials file.
— Reply to this email directly, view it on GitHub https://github.com/NOAA-GSL/VxIngest/pull/341#issuecomment-1973737388, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGDVQPVIY3NPDAWUONGDFJ3YWDEBPAVCNFSM6AAAAABECE4O5KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZTG4ZTOMZYHA . You are receiving this because you were assigned.Message ID: @.***>
-- Randy Pierce
Package | Line Rate | Branch Rate | Health |
---|---|---|---|
vxingest | 33% | 38% | ❌ |
vxingest.builder_common | 27% | 15% | ❌ |
vxingest.ctc_to_cb | 12% | 1% | ❌ |
vxingest.grib2_to_cb | 13% | 1% | ❌ |
vxingest.netcdf_to_cb | 13% | 1% | ❌ |
vxingest.partial_sums_to_cb | 12% | 1% | ❌ |
vxingest.utilities | 30% | 32% | ❌ |
Summary | 16% (490 / 3033) | 6% (40 / 712) | ❌ |
Gotcha. Thanks!
We need to cordinate this with changing the credentials files on adb-cb1, and on ascend-test2. There is an example of the proper credentials on adb-cb1 in /home/amb-verif/credentials