Skip to content

Fix BIGQUERY_JOB_DETAILS_LINK_FMT in BigQueryConsoleLink #31457

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.

Sign up for GitHub

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

Jump to bottom
Merged
eladkal merged 2 commits into from
Jun 15, 2023
Merged

Fix BIGQUERY_JOB_DETAILS_LINK_FMT in BigQueryConsoleLink #31457

eladkal merged 2 commits into from
Jun 15, 2023

Conversation

@bkossakowska
Copy link
Contributor

@bkossakowska bkossakowska commented May 22, 2023 •
edited
Loading

This PR fixes BIGQUERY_JOB_DETAILS_LINK_FMT in BigQueryConsoleLink.
The "BigQuery Console" button had an error "Invalid value for location: undefined is not a valid value".
Parameters should be passed: project_id:location:job_id instead of job_id

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:providers provider:google Google (including GCP) related issues labels May 22, 2023
@uranusjr
Copy link
Member

uranusjr commented May 22, 2023

Please provide motivations to the changes and add acompanying tests.

@bkossakowska
Copy link
Contributor Author

bkossakowska commented May 22, 2023 •
edited
Loading

Hi @potiuk !
Could you please take a look at this PR or add someone here to review changes?
Thanks!

@bkossakowska bkossakowska force-pushed the bq_links branch 2 times, most recently from 39a53b0 to acad221 Compare May 26, 2023 09:13
shahar1
shahar1 reviewed Jun 2, 2023
View reviewed changes
Copy link
Contributor

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

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

Overall LGTM - had some small comments.
CC: @eladkal

@bkossakowska bkossakowska force-pushed the bq_links branch 2 times, most recently from 9c27b7f to 8e12705 Compare June 9, 2023 13:18
@eladkal eladkal requested a review from shahar1 June 9, 2023 14:02
shahar1
shahar1 reviewed Jun 13, 2023
View reviewed changes
Copy link
Contributor

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, one last comment to take care of and you're done

@bkossakowska
Copy link
Contributor Author

bkossakowska commented Jun 14, 2023

Hi @shahar1, @potiuk
thanks for your comments, changes have been added. Is there anything else I can improve?

shahar1
shahar1 approved these changes Jun 14, 2023
View reviewed changes
Copy link
Contributor

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

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

Good job!

@eladkal eladkal changed the title Fix BIGQUERY_JOB_DETAILS_LINK_FMT in BigQueryConsoleLink Fix BIGQUERY_JOB_DETAILS_LINK_FMT in BigQueryConsoleLink Jun 15, 2023
@eladkal eladkal merged commit c7072c0 into apache:main Jun 15, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Jun 15, 2023
@potiuk
Revert "Fix BIGQUERY_JOB_DETAILS_LINK_FMT in BigQueryConsoleLink (a…
8072780 
…pache#31457)"

This reverts commit c7072c0.
@potiuk potiuk mentioned this pull request Jun 15, 2023
Merged
ephraimbuddy pushed a commit that referenced this pull request Jun 15, 2023
@potiuk
Revert "Fix BIGQUERY_JOB_DETAILS_LINK_FMT in BigQueryConsoleLink (#…
e6960f1 
…31457)" (#31935)

This reverts commit c7072c0.
@potiuk
Copy link
Member

potiuk commented Jun 15, 2023

Hey @bkossakowska - we need to revert this one for now - seems that it caused error in API tests (one of the API tests failed with bigquery link retrieval) and started failing main. You will need to re-open this one -then we will apply "full tests needed" and you will be able to fix the problem.

@eladkal
Copy link
Contributor

eladkal commented Jun 15, 2023 •
edited
Loading

sorry for the trouble @bkossakowska - our test matrix make some assumptions (so we won't always run all tests due to costs and time) so sometimes while it gives green build but actually hides an error.

As @potiuk suggested please start a new PR with the same code modifications we will make sure it runs with full tests and then you will be able to modify the code to make it work.

@bkossakowska
Copy link
Contributor Author

bkossakowska commented Jun 16, 2023

@potiuk, @eladkal
I opened a new PR: #31953,
I found the tests mentioned in tests/api_connexion/endpoints/test_extra_link_endpoint.py,
these tests have been fixed in this PR

@bkossakowska bkossakowska mentioned this pull request Jun 16, 2023
Merged
cjames23 pushed a commit to cjames23/airflow that referenced this pull request Jun 16, 2023
@bkossakowska @eladkal @cjames23
Fix BIGQUERY_JOB_DETAILS_LINK_FMT in BigQueryConsoleLink (apache#…
6cb89ed 
…31457)

Co-authored-by: Beata Kossakowska <bkossakowska@google.com>
Co-authored-by: eladkal <45845474+eladkal@users.noreply.github.com>
cjames23 pushed a commit to cjames23/airflow that referenced this pull request Jun 16, 2023
@potiuk @cjames23
Revert "Fix BIGQUERY_JOB_DETAILS_LINK_FMT in BigQueryConsoleLink (a…
34fe3b5 
…pache#31457)" (apache#31935)

This reverts commit c7072c0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eladkal eladkal eladkal approved these changes

@shahar1 shahar1 shahar1 approved these changes

Assignees

No one assigned

Labels

area:providers provider:google Google (including GCP) related issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@bkossakowska @uranusjr @potiuk @eladkal @shahar1