Fixes: GetTags-403 Permission denied on resource project None.
#14521
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Takahiro Nakayama <civitaspo@gmail.com>
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
civitaspo
commented
Dec 29, 2023
Comment on lines
+113
to
+114
| if engine.url.host: | ||
| taxonomy_project_ids.append(engine.url.host) |
There was a problem hiding this comment.
I believe that the engine.url.host does not contain the GCP ProjectID, and therefore, it might be acceptable to remove this backward compatibility. If the OpenMetadata team agrees with this perspective, please let me know, and I can make the necessary changes in this Pull Request.
There was a problem hiding this comment.
@civitaspo Thanks for the PR!
I had a question around connection to Bigquery..
how are you connecting to GCP? is it via service account credentials ( json file ) location, cli auth or is it via adding the service account details in UI?
There was a problem hiding this comment.
@ayush-shah Thank you for your response.
I am not using the UI. Instead, I am specifying a configuration YAML for ingestion/profile in the metadata CLI and executing metadata ingestion and profiling through it.
|
I considered adding tests for this change, but I was unsure where and how to write tests related to BigQuery's test_connection. If there is a reference implementation that could guide me, please let me know. If the policy is that minor changes do not require tests, I would be grateful if you could review it as is. |
harshach
added
the
safe to test
|
@civitaspo thanks for the PR. @pmbrull or @ayush-shah can we review and merge it in |
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
ayush-shah
requested changes
Dec 29, 2023
There was a problem hiding this comment.
@civitaspo can you run make py_format py_format_check to resolve the formatting issue?
Signed-off-by: Takahiro Nakayama <civitaspo@gmail.com>
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
…2:4: R1710: Either all return statements in a function should return an expression, or none of them should. (inconsistent-return-statements)" Signed-off-by: Takahiro Nakayama <civitaspo@gmail.com>
|
@civitaspo looks like it's failing for this Can you take a look at the above? Let me know if you need help around it, Thanks! |
|
@ayush-shah Thanks for the lightning-fast response! And sorry for not fixing everything in one commit! I've made the corrections now, so could you check again after the CI completes? On a different note, it seems that black and pylint are indicating the need to modify files that I haven't changed in this round, but since that would increase the volume of changes significantly, I'm thinking of making those modifications in a separate PR! black logs
PyLint logs
I believe these files depend on files generated from the schema, so rather than correcting them as PyLint, I'm thinking it would make sense to ignore the lint rules for them. |
|
@civitaspo yes, on it. For the black and pylint, can you check if the versions for those are as per the requirements? |
|
|
@ayush-shah Thank you for your comment. As you mentioned, using the version of black specified in setup.py ("black==22.3.0") resolved the check failure. Regarding pylint, even with pylint==3.0.3, the check fails locally. However, since nothing is showing up on GitHub Actions, it's likely an issue with my environment. Now that I know it's a problem with my environment, please disregard it for the moment! |
ayush-shah
approved these changes
Dec 29, 2023
|
Thanks for the contribution 🙌 |
|
It was a very responsive and positive contribution experience. Thank you very much. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your changes:
Fixed an issue in the BigQuery
test_connectionprocess where aGetTags-403 Permission denied on resource project None.error was encountered during the execution oftest_tags. The previous implementation erroneously executedlist_taxonomieswithout utilizing the specifiedtaxonomyProjectID, leading to the error. While I am uncertain ifengine.url.hostcan contain a GCP Project ID, I have made efforts to maintain backward compatibility as much as possible. However, there is one backward-incompatible change regarding the behavior whentaxonomyLocationis not set. To prevent errors whentaxonomyLocationis not configured, I have altered the process to log an info message and skip the execution oflist_taxonomies.Ingestion Error Log
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>