Skip to content
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

Make porting tiledb_unit integration tests to REST CI easier #4678

Merged
merged 20 commits into from
Mar 27, 2024

Conversation

ypatia
Copy link
Member

@ypatia ypatia commented Jan 29, 2024

This PR groups some improvements on testing infra for REST tests:

Note: REST-CI runner needs to be adapted after this is merged, but it's in a separate repo if I understand well. Also an extra runner will be added to test Query v3 that will be setting use_refactored_array_open_and_query_submit config to true by default and run all [rest] tests.


TYPE: NO_HISTORY
DESC: Make porting tiledb_unit integration tests to REST CI easier

Copy link

@ypatia ypatia force-pushed the yt/sc-37953/rest_ci_query_v3_testing_part1 branch from 0245549 to a20b900 Compare January 31, 2024 09:15
@teo-tsirpanis
Copy link
Member

Please ask me to review this when it is ready.

@ypatia ypatia marked this pull request as ready for review February 8, 2024 14:01
@ypatia ypatia marked this pull request as draft February 8, 2024 14:01
@ypatia ypatia force-pushed the yt/sc-37953/rest_ci_query_v3_testing_part1 branch from 5945774 to 37dbdca Compare February 8, 2024 14:53
@ypatia ypatia marked this pull request as ready for review February 8, 2024 14:54
@ypatia ypatia force-pushed the yt/sc-37953/rest_ci_query_v3_testing_part1 branch 2 times, most recently from 514b9a5 to a1c4a39 Compare February 9, 2024 13:08
@ypatia ypatia marked this pull request as draft February 9, 2024 13:40
@ypatia ypatia force-pushed the yt/sc-37953/rest_ci_query_v3_testing_part1 branch 5 times, most recently from 872f8fd to 08752f5 Compare February 13, 2024 13:42
@ypatia ypatia changed the title WIP: REST CI testing infra for Query v3 Make porting tiledb_unit integration tests to REST CI easier and port some tests Feb 13, 2024
@ypatia ypatia changed the title Make porting tiledb_unit integration tests to REST CI easier and port some tests Make porting tiledb_unit integration tests to REST CI easier Feb 13, 2024
@ypatia ypatia marked this pull request as ready for review February 13, 2024 14:01
Copy link
Contributor

@shaunrd0 shaunrd0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once CI is green, nice work 👍 REST CI will fail since this PR removes bootstrap flags being used in those workflows.

If you want to test this PR against TileDB-REST-CI we can update the ref parameter in ci-rest.yml to point to a different branch on TileDB-REST-CI. We would definitely want to revert that change before merging though. If you would like to test this way and run into issues there or want me to spin up a branch on TileDB-REST-CI for testing LMK, happy to help.

test/support/src/helpers.cc Outdated Show resolved Hide resolved
Comment on lines 2933 to 2937
if (uri.is_tiledb()) {
throw std::invalid_argument(
"Getting the encryption type of remote arrays is not supported.");
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we return no encryption instead of failing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KiterLuc opinion on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should keep logic out of the C-api. Can this be moved into the C++ side?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of a better place to have this logic. In the past this could fit in a StorageManager::get_array_encryption_type function, but we want to stop using StorageManager.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved it to array_get_encryption for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh there was already such method, great.

test/support/src/vfs_helpers.h Outdated Show resolved Hide resolved
test/support/src/vfs_helpers.h Outdated Show resolved Hide resolved
@ypatia ypatia force-pushed the yt/sc-37953/rest_ci_query_v3_testing_part1 branch from 4249e85 to 3cccacb Compare February 14, 2024 14:19
@ypatia ypatia force-pushed the yt/sc-37953/rest_ci_query_v3_testing_part1 branch 4 times, most recently from 21b3d8b to 7d128ca Compare February 21, 2024 10:46
@ypatia
Copy link
Member Author

ypatia commented Feb 21, 2024

LGTM once CI is green, nice work 👍 REST CI will fail since this PR removes bootstrap flags being used in those workflows.

If you want to test this PR against TileDB-REST-CI we can update the ref parameter in ci-rest.yml to point to a different branch on TileDB-REST-CI. We would definitely want to revert that change before merging though. If you would like to test this way and run into issues there or want me to spin up a branch on TileDB-REST-CI for testing LMK, happy to help.

Thanks for the idea and the pointers! I did that (and reverted) against my Tiledb-REST-CI branch: https://github.com/TileDB-Inc/TileDB-REST-CI/pull/17
And it passes: https://github.com/TileDB-Inc/TileDB-REST-CI/actions/runs/7987300561/job/21809494707 .
So, as soon as CI of the current PR passes we are good to merge both. FYI: @KiterLuc

Comment on lines +781 to +787
if (uri.is_tiledb()) {
throw std::invalid_argument(
"Getting the encryption type of remote arrays is not supported.");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (uri.is_tiledb()) {
throw std::invalid_argument(
"Getting the encryption type of remote arrays is not supported.");
}
if (uri.is_tiledb()) {
*encryption_type = EncryptionType::NO_ENCRYPTION;
return Status::Ok();
}

@ypatia ypatia force-pushed the yt/sc-37953/rest_ci_query_v3_testing_part1 branch 3 times, most recently from f2be9f8 to e49182f Compare February 27, 2024 13:47
@KiterLuc KiterLuc force-pushed the yt/sc-37953/rest_ci_query_v3_testing_part1 branch from c472fa7 to 12a46cb Compare March 1, 2024 12:43
@ypatia ypatia force-pushed the yt/sc-37953/rest_ci_query_v3_testing_part1 branch from c4a5e57 to ecaec9c Compare March 27, 2024 13:21
@KiterLuc KiterLuc merged commit ea6a0e7 into dev Mar 27, 2024
58 of 59 checks passed
@KiterLuc KiterLuc deleted the yt/sc-37953/rest_ci_query_v3_testing_part1 branch March 27, 2024 15:23
Shelnutt2 added a commit that referenced this pull request Apr 22, 2024
This passes the `cloud_rest_ref` parameter to pin to a 2.18 compatible
version so that the compilation and linking works as expected, including
using the older pre-vfs option that came in #4678 .

---
TYPE: NO_HISTORY
DESC: pin ci-rest job to 2.18 compatible version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants