Align the default version with Facebook business SDK

[RyanSiu1995]

Upgrade the graph api to latest version

Closes #18845
No test needs to be updated because the parameters are supplied from caller

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

[mik-laj]

It looks like a breaking change. Can you add a note in the changelog?

[eladkal]

I think it would be better not to set our own default. We can import the default value from the library itself
https://github.com/facebook/facebook-python-business-sdk/blob/fbc3b6a5c2d54bd4184163b290dd614b668e4ee0/facebook_business/apiconfig.py#L22

or maybe better just not have a default value for api_version at all.
The upstream lib made this parameter optional:
https://github.com/facebook/facebook-python-business-sdk/blob/fbc3b6a5c2d54bd4184163b290dd614b668e4ee0/facebook_business/api.py#L181
That way it will use the default value of the upstream lib if the user didn't provide a value.

[RyanSiu1995]

Instead of dropping the default value, why not making it as Optional?
User can still define their own version.
But the library will fall back to facebook_businss default api version if they don't specifiy it.

[eladkal]

sorry if wasn't clear - this is what I meant.
Just treat it like any other optional param with default of None.

[eladkal]

This is not the place for it.
The note should be in the provider change log.
see https://github.com/apache/airflow/pull/18493/files for example

[mik-laj]

I'm glad you want to write a new note. Remember that this note is intended for users.
Make sure it contains the following information:

• Previous behaviors
• New behaviors
• If possible, a simple example of how to migrate. This may include a simple code example.
• If possible, the benefit for the user after migration e.g. "we want to make these changes to unify class names."
• If possible, the reason for the change, which adds more context to that interest, e.g. reference for Airflow Improvement Proposal or external documentation.

More tips can be found in the guide:
https://developers.google.com/style/inclusive-documentation

[potiuk]

For now - for provider changes we do NOT expect anyone to change any changelogs - until we introduce a tool like towncrier, making any rules like that makes more harm than good - mainly because we have:
a) no agreed rules (quoting google here is nice but I did not recall we took that as a base)
b) we have no agreement between committers on different rules
c) the release manager of provider (currently mysefl) prepares the changelog.

I do not think we have any rules, more I think introducing any rules, requires introducing protection and verification that those rules are followed (otherwise some changes will have it but others will not) - so unless we document the rules we should follow and have some way for the committers to make sure the commenrs are added, I think it makes little sense to ask people to follow such rules which are not documented/agreed.

I think for now it's best to remove the changelog entries, and leave it up to release manger, but it would be great if someone introduce automation and verification (I will do it it November, but maybe someone wants to do it earlier).

[mik-laj]

So where do we want to describe the breaking changes? I am afraid that the title alone is not enough in many cases. The task manager also often does not have enough knowledge to prepare a description of the change that is sufficient, because the git diff often does not show the full context. The preparation of the note also forces us to reflect on whether we really need to introduce such a breaking change.(see example: #18879 (comment))
Regarding the content of the notes about breaking time, this has been discussed in the past, and we have some hints in the main UPDATING.md file.

airflow/UPDATING.md

Lines 64 to 78 in ab5b2bf

	<!--
	I'm glad you want to write a new note. Remember that this note is intended for users.
	Make sure it contains the following information:
	- [ ] Previous behaviors
	- [ ] New behaviors
	- [ ] If possible, a simple example of how to migrate. This may include a simple code example.
	- [ ] If possible, the benefit for the user after migration e.g. "we want to make these changes to unify class names."
	- [ ] If possible, the reason for the change, which adds more context to that interested, e.g. reference for Airflow Improvement Proposal.
	More tips can be found in the guide:
	https://developers.google.com/style/inclusive-documentation
	-->

Here is a related discussion: #18879 (comment)

[potiuk]

So where do we want to describe the breaking changes? I am afraid that the title alone is not enough in many cases.

Sure - we need to figure it out. It's not done yet. We need:

1. a way to do it
2. a way to flag if not done for all PR
3. a documentation for contributors to follow
4. a way to automate it to bring it to changelog automatically

Until it's not done, it's just random advice, because no-one (including other committers) will follow it.

I think about implementing towncrier for that and documenting it, but until it is done, I think either someone does it (all points) or we just follow the currnet process (where release manager does it), because we have no documentation for it (unless you tell me otherwise and documentation is there)?

[potiuk]

You refer to single change done by you in Jan. Since then we had 7 releases of providers which followed different proces and every one of them would have taken much longer time for coordination, automation if it was not automated how it is today.

The thing you added then was a single case that did not establish a proces, way how to do it for scaled number of providers and how to make sure it is followed. As such - it does not really establishes a "process".
It's an accidental change that is dificutlt to follow in scale, there is no automation to enforce it, nor maintain, and none of the committers is even aware what should they tell others when then review a change if they were to follow it. If it just "done" but not agreed, documented and neither "automation" is in place to kee'p it nor people involved know that it should be followed.

Until this is done, this does not exist as a rule. It's just a "single change'.

It was ok for 1 provider and something you added and it was never documented how to do it.

Since then there were 6 or 7 waves of providers which were followed using different process which means I can release 60 providers in maye 1-2 hours. You are now trying to make a change to it wihich potentially changes it to 6 hours because relase manager will have to make extra checks on every single change and reconcile the changelog manually and make sure that every single change has been either manually added already or automatically incorporated into changelog.

If you reallyy want to fight for it and change the current process - I am perfectly ok with that - there is a next wave of providers to release in October. If you would like to take a lead on that and release them, I am all for changing it now and won't object changing the current approach.

Are you ok with it? Can we count on it?

[mik-laj]

I did not move the migration guides from a separate file to the changelogs.

This change has been committed by @potiuk on 1 Feb.

For the sake of clarity, I don't want to change the process now. I just wanted to understand what has changed since we made "a one change" that has structured the package documentation until now. Now I understand that we just gained experience and discovered the weaknesses of the current process. I'm totally fine with that.

[mik-laj]

I think we now have a continuation of this thread.
#13767 (comment)
I have nothing to add, nor do I want to change anything in the process now. I am also not going to ask for manual editing in the CHANGELOG.rst file.

[RyanSiu1995]

Hi all, I am going to remove that from CHANGELOG.rst and let it alone, right?

[potiuk]

pleease :). I think we agreed with @mik-laj finally ;). Sorry for being dragged into such discussion. We are both passionate about it and sometimes I have to strong of an opinion :D

[github-actions]

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

[ageofneil]

Are we sure this is working as intended?

Traceback (most recent call last):
  File "/home/airflow/.local/lib/python3.6/site-packages/airflow/models/taskinstance.py", line 1157, in _run_raw_task
    self._prepare_and_execute_task_with_callbacks(context, task)
  File "/home/airflow/.local/lib/python3.6/site-packages/airflow/models/taskinstance.py", line 1331, in _prepare_and_execute_task_with_callbacks
    result = self._execute_task(context, task_copy)
  File "/home/airflow/.local/lib/python3.6/site-packages/airflow/models/taskinstance.py", line 1361, in _execute_task
    result = task_copy.execute(context=context)
  File "/home/airflow/.local/lib/python3.6/site-packages/airflow/providers/google/cloud/transfers/facebook_ads_to_gcs.py", line 111, in execute
    rows = service.bulk_facebook_report(params=self.params, fields=self.fields)
  File "/home/airflow/.local/lib/python3.6/site-packages/airflow/providers/facebook/ads/hooks/ads.py", line 125, in bulk_facebook_report
    _async = ad_account.get_insights(params=params, fields=fields, is_async=True)
  File "/home/airflow/.local/lib/python3.6/site-packages/facebook_business/adobjects/adaccount.py", line 2732, in get_insights
    return self.get_insights_async(fields, params, batch, success, failure, pending)
  File "/home/airflow/.local/lib/python3.6/site-packages/facebook_business/adobjects/adaccount.py", line 2851, in get_insights_async
    return request.execute()
  File "/home/airflow/.local/lib/python3.6/site-packages/facebook_business/api.py", line 687, in execute
    api_version=self._api_version,
  File "/home/airflow/.local/lib/python3.6/site-packages/facebook_business/api.py", line 350, in call
    raise fb_response.error()
facebook_business.exceptions.FacebookRequestError: 

  Message: Call was not successful
  Method:  POST
  Path:    https://graph.facebook.com/v6.0/*****/insights
  Params:  {'level': 'ad', 'date_preset': 'yesterday', 'fields': '["campaign_name","campaign_id","ad_id","clicks","impressions"]'}

  Status:  400
  Response:
    {
      "error": {
        "message": "(#2635) You are calling a deprecated version of the Ads API. Please update to the latest version: v11.0.",
        "type": "OAuthException",
        "code": 2635,
        "fbtrace_id": "ADTlqAD1sH_MY6I2macc2Hy"
      }
    }

I'm running this code: https://airflow.apache.org/docs/apache-airflow-providers-google/stable/_modules/airflow/providers/google/cloud/example_dags/example_facebook_ads_to_gcs.html

[eladkal]

@ageofneil will be addressed in #18996