make tests work with new indexes and new credentials

[randytpierce]

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

[ian-noaa]

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.

[randytpierce]

.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 <#341 (comment)>: This shouldn't be checked in since it's user-specific. Maybe we should update the .gitignore to exclude .env* patterns? ------------------------------ On .env-cb1 <#341 (comment)>: Same comment here as the .env-capella file. ------------------------------ In src/vxingest/builder_common/ingest_manager.py <#341 (comment)>: > - 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 <#341 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGDVQPTRPIZV37OIEUYUAG3YWDAZHAVCNFSM6AAAAABECE4O5KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMJRG42DSMZVG4> . You are receiving this because you were assigned.Message ID: ***@***.***>

-- Randy Pierce

[randytpierce]

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 <#341 (comment)>: This shouldn't be checked in since it's user-specific. Maybe we should update the .gitignore to exclude .env* patterns? ------------------------------ On .env-cb1 <#341 (comment)>: Same comment here as the .env-capella file. ------------------------------ In src/vxingest/builder_common/ingest_manager.py <#341 (comment)>: > - 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 <#341 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGDVQPTRPIZV37OIEUYUAG3YWDAZHAVCNFSM6AAAAABECE4O5KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMJRG42DSMZVG4> . You are receiving this because you were assigned.Message ID: ***@***.***>

-- Randy Pierce

[ian-noaa]

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.

[ian-noaa]

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

[randytpierce]

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 <#341 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGDVQPUW2LJEJBS6TUDCA53YWDD55AVCNFSM6AAAAABECE4O5KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMJRHAYDMNZRGY> . You are receiving this because you were assigned.Message ID: ***@***.***>

-- Randy Pierce

[randytpierce]

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 <#341 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGDVQPVIY3NPDAWUONGDFJ3YWDEBPAVCNFSM6AAAAABECE4O5KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZTG4ZTOMZYHA> . You are receiving this because you were assigned.Message ID: ***@***.***>

-- Randy Pierce

[github-actions]

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)	❌

[ian-noaa]

Gotcha. Thanks!