-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Remove Google Analytics driver #43352
Conversation
junit-name: 'be-tests-googleanalytics-ee' | ||
test-args: ":only ${{ inputs.test }} :times ${{ inputs.times }}" | ||
|
||
be-google-related-drivers-classpath-test: |
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.
this job was to check that Google Analytics and Big Query dependencies didn't conflict with each other so we don't need it anymore
|
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, with comments.
@@ -21,7 +21,6 @@ on: | |||
- Athena | |||
- BigQuery | |||
- Druid | |||
- Google Analytics | |||
- Google Classpath Test |
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.
Google Classpath Test should not be removed?
@@ -20,8 +20,7 @@ | |||
"/enterprise/backend/src" | |||
"/modules/drivers/bigquery-cloud-sdk/src" | |||
"/modules/drivers/druid/src" | |||
"/modules/drivers/google/src" | |||
"/modules/drivers/googleanalytics/src" | |||
"/modules/drivers/druid-jdbc/src" |
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!
@@ -33,70 +32,6 @@ | |||
(throw (ex-info (format "Did not find %s.%s" (name table) (name column)) | |||
{:table table :column column})))) | |||
|
|||
(deftest ^:parallel field-matching-predicates-test |
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.
Why to remove whole test and not just ga:
related parts?
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.
good catch! Fixed
@camsaul Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
* Remove Google Analytics driver * Remove more GA-related tests * Un-remove tests that aren't related to GA
I think I managed to remove all the special case stuff
Resolves #42792