-
Notifications
You must be signed in to change notification settings - Fork 329
Refactor Polaris cloudTests and adopt changes from PR #2405 #2871
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
Conversation
snazy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks for reviving this!
Thank you for the quick review! 😊 |
| extends PolarisRestCatalogIntegrationBase { | ||
| public static final String TENANT_ID = System.getenv("INTEGRATION_TEST_AZURE_TENANT_ID"); | ||
| public static final String BASE_LOCATION = System.getenv("INTEGRATION_TEST_AZURE_PATH"); | ||
| public static final String TENANT_ID = System.getenv("INTEGRATION_TEST_ADLS_TENANT_ID"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrew4699 : is that change ok from your POV?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renaming this will certainly break things that depend on it, but I haven't been involved with the latest guidelines on changes that break internal "interfaces", so I'd defer to what the status quo is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into it @dimas-b & @andrew4699! Makes sense, reverted the env rename part.
dimas-b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes LGTM 👍 Thanks, @tmater !
I wonder if env. var. renames might affect some downstream projects... Let's wait a bit before merging this to allow more people to review.
|
Just checking if we’re good to merge, or if there’s anything else needed here. |
snazy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM!
Let's get this in.
Summary
This patch picks up changes from apache/polaris#2405.
It includes only the refactoring portions, as there have been updates to the
createViewWithCustomMetadataLocationUsingPolarisandcreateViewWithCustomMetadataLocationtests that require further investigation.Details
Integrated relevant refactors from upstream PR #2405, renaming classes, environment variables and making Base classes abstract.
Known Issues
The following tests are still failing in
cloudTest:createViewWithCustomMetadataLocationUsingPolariscreateViewWithCustomMetadataLocationThese will be investigated and fixed in a follow-up PR.
Testing
cloudTestslocally to validate refactor integrity.