-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
🐛Destination-bigquery: added additional verifications to Check method #18554
🐛Destination-bigquery: added additional verifications to Check method #18554
Conversation
NOTE
|
/test connector=connectors/destination-bigquery
Build FailedTest summary info:
|
/test connector=connectors/destination-bigquery-denormalized
Build PassedTest summary info:
|
/test connector=connectors/destination-bigquery
Build FailedTest summary info:
|
/test connector=connectors/destination-bigquery
Build FailedTest summary info:
|
/test connector=connectors/destination-bigquery
Build FailedTest summary info:
|
/test connector=connectors/destination-bigquery
Build FailedTest summary info:
|
NOTE
|
/test connector=connectors/destination-bigquery
Build PassedTest summary info:
|
NOTE
|
NOTE
|
/test connector=connectors/destination-bigquery-denormalized
Build PassedTest summary info:
|
@akashkulk could you please check if this change will report the error in the way that aligns with what you are working on for sources and that errors will be reported as config errors? |
...ation-bigquery/src/main/java/io/airbyte/integrations/destination/bigquery/BigQueryUtils.java
Show resolved
Hide resolved
@MethodSource("datasetIdResetterProvider") | ||
void testCheckFailureInsufficientPermissionForCreateDataset(final DatasetIdResetter resetDatasetId) throws IOException { | ||
|
||
if (!Files.exists(CREDENTIALS_WITHMISSED_CREATE_DATASET_ROLE_PATH)) { |
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.
I see this occurring in other parts of the code, what exactly is this supposed to do that is causing a test to throw an IllegalStateException
?
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 is another test creds.
I will fail on CI if somebody removes or change the secret in GSM or to somebody, who will try to run integrationTest locally without having these creds locally in the secret folder. I believe this common integration test's approach for connectors that calls real DB i.e. requires real creds that obviously can't be committed.
This exception will provide a more clear understanding what went wrong rather than just NullPointerException
Affected Connector ReportNOTE
|
Connector | Version | Changelog | Publish |
---|
- See "Actionable Items" below for how to resolve warnings and errors.
⚠ Destinations (2)
Connector | Version | Changelog | Publish |
---|---|---|---|
destination-bigquery |
1.2.6 |
✅ | ✅ |
destination-bigquery-denormalized |
1.2.6 |
⚠ (doc not found) |
✅ |
- See "Actionable Items" below for how to resolve warnings and errors.
Actionable Items
(click to expand)
Category | Status | Actionable Item |
---|---|---|
Version | ❌ mismatch |
The version of the connector is different from its normal variant. Please bump the version of the connector. |
⚠ doc not found |
The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like source-jdbc is not published or documented). Please double-check to make sure that it is not a bug. |
|
Changelog | ⚠ doc not found |
The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like source-jdbc is not published or documented). Please double-check to make sure that it is not a bug. |
❌ changelog missing |
There is no chnagelog for the current version of the connector. If you are the author of the current version, please add a changelog. | |
Publish | ⚠ not in seed |
The connector is not in the seed file (e.g. source_definitions.yaml ), so its publication status cannot be checked. This can be normal (e.g. some connectors are cloud-specific, and only listed in the cloud seed file). Please double-check to make sure that it is not a bug. |
❌ diff seed version |
The connector exists in the seed file, but the latest version is not listed there. This usually means that the latest version is not published. Please use the /publish command to publish the latest version. |
/test connector=connectors/destination-bigquery
Build PassedTest summary info:
|
.withStatus(AirbyteConnectionStatus.Status.FAILED) | ||
.withMessage("Could access the GCS bucket with the provided configuration.\n" + e | ||
.getMessage()); | ||
throw new ConfigErrorException("Could access the GCS bucket with the provided configuration.\n", e); |
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.
It seems this message should be "Could not access the GCS bucket with the provided configuration.\n", right because we're saying that it failed
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.
Hm... Interesting fact, it's been there for already almost half a year. Updated, thanks
@@ -72,6 +74,8 @@ | |||
class BigQueryDestinationTest { | |||
|
|||
protected static final Path CREDENTIALS_PATH = Path.of("secrets/credentials.json"); | |||
protected static final Path CREDENTIALS_WITHMISSED_CREATE_DATASET_ROLE_PATH = |
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.
Missing an _
between with and missed
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.
updated, thanks
}); | ||
|
||
final String actualMessage = ex.getMessage(); | ||
LOGGER.info("Checking expected failure message:" + actualMessage); |
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.
Not sure if this LOGGER
is needed. To keep the test simpler, may suggest just having this as
assertThat(ex.getMessage()).contains("Access Denied");
cc: @ChristopheDuong is there any reason we may want to keep this LOGGER.info
?
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.
Not sure if this LOGGER is needed.
probably not? just for surfacing information in the logs
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.
removed logger, thanks
final AirbyteConnectionStatus expected = new AirbyteConnectionStatus().withStatus(Status.FAILED).withMessage(""); | ||
assertEquals(expected, actual.withMessage("")); | ||
assertThat(actualMessage) | ||
.contains("Project dataline-integration-testing: User does not have bigquery.datasets.create permission"); |
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.
I would probably remove the dataline-integration-testing:
portion since that's really internal to Airbyte and better to keep this not directly exposed since it provides no value to anybody else
Also same comment as above, if the LOGGER
class is not needed then this assertThat
can be simplified into one line
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.
updated, thanks
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.
Overall looks good, some minor nits with the checkGcsPermission
message. Lets not provide an odd message but overall after that change it should be good to move forward
Looks good. Please address @ryankfu 's comments before publishing and merging. |
/test connector=connectors/destination-bigquery
Build PassedTest summary info:
|
/test connector=connectors/destination-bigquery-denormalized
Build PassedTest summary info:
|
/publish connector=connectors/destination-bigquery
if you have connectors that successfully published but failed definition generation, follow step 4 here |
/publish connector=connectors/destination-bigquery-denormalized
if you have connectors that successfully published but failed definition generation, follow step 4 here |
…#18554) * [16992] Destination-bigquery: added additional verifications to Check connection method
What
The check stage sometimes may pass the verification but then fail on sync.
How
Updated check stage - added additional verifications
Tested locally:
Test with valid creds
Test with valid GCS creds
🚨 User Impact 🚨
Existing users shouldn't be affected
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereUpdating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereConnector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changesTests
Unit
Put your unit tests output here.
Integration
Put your integration tests output here.
Acceptance
Put your acceptance tests output here.