Skip to content
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

Update DV360 operators to use API v2 #30326

Merged
merged 5 commits into from
Apr 9, 2023
Merged

Conversation

lwyszomi
Copy link
Contributor

@lwyszomi lwyszomi commented Mar 27, 2023

Support latest version of Display Video 360 API, because v1 and v1.1 will be sunset soon.


^ 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 kind:documentation provider:google Google (including GCP) related issues labels Mar 27, 2023
@lwyszomi
Copy link
Contributor Author

@potiuk @eladkal Can we include this in the next provider release? I think sunset of the API will be in the middle of April

@eladkal
Copy link
Contributor

eladkal commented Mar 27, 2023

Does this PR closes #28045 ?

@lwyszomi
Copy link
Contributor Author

@eladkal most important part yes, regarding queries/reports

@@ -23,6 +23,15 @@
Changelog
---------

9.0.0
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if our tooling will break when we get to 10 here. Let's see 🤔

Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

Something is odd.
I would have expected to CI complain about need to add deprecated operators to:

DEPRECATED_CLASSES = {
"airflow.providers.google.cloud.operators.cloud_storage_transfer_service"
".CloudDataTransferServiceS3ToGCSOperator",
"airflow.providers.google.cloud.operators.cloud_storage_transfer_service"
".CloudDataTransferServiceGCSToGCSOperator",
"airflow.providers.google.cloud.operators.dataproc.DataprocSubmitHadoopJobOperator",
"airflow.providers.google.cloud.operators.dataproc.DataprocScaleClusterOperator",
"airflow.providers.google.cloud.operators.dataproc.DataprocSubmitSparkJobOperator",
"airflow.providers.google.cloud.operators.dataproc.DataprocSubmitSparkSqlJobOperator",
"airflow.providers.google.cloud.operators.dataproc.DataprocSubmitHiveJobOperator",
"airflow.providers.google.cloud.operators.dataproc.DataprocSubmitPigJobOperator",
"airflow.providers.google.cloud.operators.dataproc.DataprocSubmitPySparkJobOperator",
"airflow.providers.google.cloud.operators.mlengine.MLEngineManageModelOperator",
"airflow.providers.google.cloud.operators.mlengine.MLEngineManageVersionOperator",
"airflow.providers.google.cloud.operators.dataflow.DataflowCreateJavaJobOperator",
"airflow.providers.google.cloud.operators.bigquery.BigQueryPatchDatasetOperator",
"airflow.providers.google.cloud.operators.dataflow.DataflowCreatePythonJobOperator",
"airflow.providers.google.cloud.operators.bigquery.BigQueryExecuteQueryOperator",
}

@potiuk
Copy link
Member

potiuk commented Mar 29, 2023

Something is odd. I would have expected to CI complain about need to add deprecated operators to:

@pytest.mark.xfail(reason="We did not reach full coverage yet")

xfail = "expected to fail"

Not sure who marked it as such, but that's the reason it did not fail the build

Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

LGTM

@lwyszomi lwyszomi force-pushed the dv360-update branch 2 times, most recently from 1eda3cb to eb682a9 Compare April 3, 2023 06:47
@eladkal
Copy link
Contributor

eladkal commented Apr 7, 2023

doc build is failing

@potiuk
Copy link
Member

potiuk commented Apr 8, 2023

I (think) fixed the error and rebased it, so that we can merge it and release in the ad-hoc release of Google Provider (cc: @eladkal )

@potiuk
Copy link
Member

potiuk commented Apr 8, 2023

cc: @lwyszomi ^^

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure the doc build is failing because this PR adds new operators but they are not listed in provider.yaml
can you please check it?

Copy link
Member

Choose a reason for hiding this comment

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

That was not the reason.

Operators are never listed "individually" in provider.yaml - only Hooks are listed when they are contributing new connection types.

The problems

I fixed the build and there were three problems:

  1. there was an apostrophe instead of backtick around the http link added:
`https://developers.google.com/bid-manager/v2/queries/delete'

Should be:

`https://developers.google.com/bid-manager/v2/queries/delete`

Small difference and not easy to spot the error, but it was enough to make sphinx generation freak-out and produce error messages that were completely misleading.

  1. There is still a bit problem with the new links generated (some of them are copy&pasted

Solutions

So I applied three solutions:

  1. replaced the ' with backtick
  2. i manually removed the anchors to stop the docs from failing but the final fix is stil waiting for @lwyszomi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@potiuk you are the god, how you found this apostrophe, I tried to understand whole friday why it failing and why error is shown in the generated files without any "real" suggestion. Sorry I was whole weekend offline, Easter holidays. Today I will prepared needed changes.

Copy link
Member

Choose a reason for hiding this comment

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

:D I am just intimately familiar with Sphinxes :) and I know they speak in riddles.

And yes it took me quite a bit of bisecting to find out (this is the technique I often use when I am at a loss and remove parts of the commits to narrow down the search.

What also helped is #30555 and I think we should - at the very least make it possible to have incremental builds for the docs (only changed parts) - but I referained from diggung into it as we are likely to improve the whole stack of docs building HOPEFULLY soon. Or maybe just allow partial generation of only subset of provider. That could have helped with bisecting and iterations immensely in such cases.

@potiuk potiuk merged commit 71db47a into apache:main Apr 9, 2023
kosteev pushed a commit to kosteev/composer-airflow-test-copybara that referenced this pull request Sep 12, 2024
Release google provider package v2023.4.3:
apache/airflow#30326
apache/airflow#29234

Change-Id: I03928b0d940b710ff40d6c6de61519f99954dbee
GitOrigin-RevId: eb05386e3782b85dd1bcd11127ecf2204c1751ff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers kind:documentation provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants