Remove Tableau from Salesforce provider #23747

eladkal · 2022-05-17T12:28:36Z

Tableau was split from Salesforce provider a year+ ago in #14030
We kept Tableau in Salesforce to preserve backward compatibility, it's time to remove the deprecated dependency.

This is a breaking change for Salesforce provider.
I didn't place entry in changelog as it caused problems in the past for the automatic release process @potiuk please let me know if you prefer otherwise

^ Add meaningful description above

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 a newsfragement file, named {pr_number}.significant.rst, in newsfragments.

jhtimmins · 2022-05-20T06:59:08Z

@potiuk @ashb Since this is a breaking change for the salesforce provider, does that mean we can't merge until 4.0?

eladkal · 2022-05-20T07:33:17Z

We can because this is a provider relase not Airflow core.

ashb · 2022-05-20T13:49:17Z

docs/apache-airflow-providers-salesforce/index.rst

 =======================  ==================

-Cross provider package dependencies


Is it worth keeping a note here saying "want tableu? It's moved"?

That is covered by the change log of the provider :)

If someone is currently using Tableau via Salesforce - the user is aware it's deprecated by warnings and will know it's removed by reading the change log.

If someone never used Tableau before and searches for tableau provider I want him/her to land in Tableau docs rather than Salesforce docs only to be referred to Tableau provider.

This is all good in general (but needs a note in CHANGELOG). We have "addional extra" - if somoene wants tableau they should install salesforce provider with ["tableau"] extra (or install [tableau]' extra in airflow or install apache-airflow-providers-extra'. But I agree we should add a note in the chanelog of salesforce. And answering the question of @ashb below - this is described actually in changelog.rst of each provider (kind of preparation to make them fully independent and splittable):

https://raw.githubusercontent.com/apache/airflow/main/airflow/providers/salesforce/CHANGELOG.rst

.. NOTE TO CONTRIBUTORS: Please, only add notes to the Changelog just below the "Changelog" header when there are some breaking changes and you want to add an explanation to the users on how they are supposed to deal with them. The changelog is updated and maintained semi-automatically by release manager.

Hmm. Looking at the build image failure in the PR. looks strange.

added note 19f6ace

ashb

LGTM, I can't remember what changes we need to make now to mark this as needing a major ver bump of the provider when we next release it though.

github-actions · 2022-05-20T13:50:58Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

eladkal · 2022-05-20T14:47:33Z

LGTM, I can't remember what changes we need to make now to mark this as needing a major ver bump of the provider when we next release it though.

Jarek handles it during release process.
He sets the proper version and create the entries in change log.

I'm holding merge on this PR till we decide on policy for removing deprecated code from providers
Mailing list thread
https://lists.apache.org/thread/6px7f0r3mmocbox45prlso7zor7wzjhx

eladkal · 2022-06-21T20:46:47Z

PR is waiting for discussion on https://lists.apache.org/thread/6ngq79df7op541gfwntspdtsvzlv1cr6 to be finished and probably a voting to pass.

eladkal · 2022-06-30T08:09:02Z

generated/provider_dependencies.json

-      "tableauserverclient"
    ],
-    "cross-providers-deps": [
-      "tableau"
-    ]
+    "cross-providers-deps": []


@potiuk can you confirm this is right?
wasn't sure with the recent change of removing provider dependencies from core.
Did I miss something else needed?

OK. Got it. This is wrong:

"simple-salesforce>=1.0.0", ], "cross-providers-deps": []

Json does not allow comma at the end of the list (after simple-salesforce). This is why "Build CI" image is failing - when you look at the error raised in build CI you will see this:

#69 1.592 Exception while loading provider dependencies Expecting value: line 555 column 5 (char 11592)

This is the error from json parser complaining about the comma.

This is an interesting one and a good learning/opportunity to improve this part.

You should NOT modify this file by hand. It should be updated automatically by pre-commit. I specifically put it into "generated" folder and added a README there to explain this (because json does not support comments, so you cannot really explain it in the file). But I entirely see (and I expected that) that people will attempt to modify it (this is why I put it in the "generated" folder. to indicate you should not modify it.

The pre-commits had no chance to work because of json parsing at image build time (inside setup.py). Otherwise you would likely figure out that you should not modify it, because you would see pre-commit failing while regenerating the file and showing you generated diff in the CI. This did not happen - because CI image failed to build because of json parsing error. This is a bit chicken-egg problem if someone modifies the .json in the way that fails parsing.

Proposals how to improve (besides fixing it):

I can simply parse the .json outside of the setup.py as part of the CI job building the image. Then I could provide a very explicit message "PLEASE DO NOT modify this file".

I can also run only that pre-commit before building the image to regenerate it. This might be nicer because it will not stop the image from building (the regenerated .json file will be ALWAYS good because it is well, generated. Then tthe whole build and test process will continue - but static checks will fail when running the same pre-commit in the "CI" workflow. - > I think this is best way

Does any of you have any better idea how to signal users they should not modify this file :) @ashb @kaxil @eladkal :). I thought I went a bit above and beyond but even that did not prevent a committer and PMC member to THINK they should modify it (and the day after it's been merged) :). I am not sure if I have any more ideas :P

@potiuk I modified this file only because pre commit asked me to I didnt even know about this file before the pre commit error - this is why I asked you it was strange to me that the error came from pre commit

oh wait I read your message wrong.
I think what we have now is OK - I edited it manually because I had issue with pre-commits.
i think we just need to add to the pre-commit message also the sentence "Don't fix manually, run the pre commit to automatically handle this"

Yeah. Makes sense

potiuk · 2022-06-30T10:34:42Z

Jarek handles it during release process. He sets the proper version and create the entries in change log.

Well. Not entirely :). It's been slighty adapted recently - see the note in Changelog. Similarly as in case of "newsfragments" the contributors should add notes to Changelog when there are breaking changes. I thought about implementing newsframents for this too, but this is totally not worth the hassle - they make sense for Airflow, where a lot of various contributors have potentially breaking changes for various areas, tracking the newsfragments and maklng them cherry-pickable together with PR makes more sense. For providers, those are far more localized and easier to track, so adding newsfragments there makes little sense (though after we split providers, we might want to implement newsfragments for say - google provider which has potentially multiple contributors working in parallell on several breaking changes.

potiuk · 2022-06-30T16:29:02Z

All right - turns that we might have a bit of a problem with building ARM images when cache is not refreshed (or when there are too many changes in Dockerfie/python deps - our machine has 50 minutes time of life and building in parallel 4 ARM images on it "from almost scratch" - I will have to increase the time of life for the ARM instance a bit :)

potiuk · 2022-06-30T16:43:47Z

Fix for that one here: #24765

Tableau was split from Salesforce provider a year+ ago in apache#14030 We kept Tableau in Salesforce to preserve backward compatibility, it's time to remove the deprecated dependency.

potiuk · 2022-06-30T21:49:57Z

Rebased to check if the timeout fix for ARM image wil work here :)

eladkal · 2022-07-03T16:31:12Z

Now that lazy consensus passed
https://lists.apache.org/thread/sthhjwd90xnmokyk4m15hcxgwdv38j4j
and #24680 is merged with updated policy
We can proceed to merge this PR.

eladkal requested a review from mik-laj as a code owner May 17, 2022 12:28

boring-cyborg bot added area:providers kind:documentation labels May 17, 2022

eladkal force-pushed the sf branch from 100fd83 to 2a38a4c Compare May 17, 2022 13:47

eladkal requested review from potiuk and ashb as code owners May 17, 2022 14:31

ashb reviewed May 20, 2022

View reviewed changes

ashb approved these changes May 20, 2022

View reviewed changes

github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label May 20, 2022

kaxil approved these changes May 22, 2022

View reviewed changes

eladkal force-pushed the sf branch from 4639a80 to 3324d00 Compare June 21, 2022 20:41

eladkal force-pushed the sf branch 4 times, most recently from 6c440bd to bf162d7 Compare June 30, 2022 08:07

eladkal commented Jun 30, 2022

View reviewed changes

eladkal added 3 commits June 30, 2022 23:49

Remove Tableau from Salesforce provider

e2f834d

Tableau was split from Salesforce provider a year+ ago in apache#14030 We kept Tableau in Salesforce to preserve backward compatibility, it's time to remove the deprecated dependency.

add note in CHANGELOG.rst

905e033

fix error

d83673d

potiuk force-pushed the sf branch from 22c70ec to d83673d Compare June 30, 2022 21:49

eladkal merged commit 9c99552 into apache:main Jul 3, 2022

eladkal deleted the sf branch July 3, 2022 16:33

eladkal mentioned this pull request Jul 13, 2022

Add documentation for July 2022 Provider's release #25030

Merged

potiuk mentioned this pull request Jul 13, 2022

Status of testing Providers that were prepared on July 13, 2022 #25037

Closed

95 tasks

ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Aug 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Tableau from Salesforce provider #23747

Remove Tableau from Salesforce provider #23747

eladkal commented May 17, 2022

jhtimmins commented May 20, 2022

eladkal commented May 20, 2022

ashb May 20, 2022

eladkal May 20, 2022

potiuk Jun 30, 2022 •

edited

Loading

potiuk Jun 30, 2022

eladkal Jun 30, 2022

ashb left a comment

github-actions bot commented May 20, 2022

eladkal commented May 20, 2022

eladkal commented Jun 21, 2022

eladkal Jun 30, 2022

potiuk Jun 30, 2022 •

edited

Loading

eladkal Jun 30, 2022 •

edited

Loading

eladkal Jun 30, 2022

potiuk Jun 30, 2022

potiuk Jun 30, 2022

potiuk commented Jun 30, 2022

potiuk commented Jun 30, 2022

potiuk commented Jun 30, 2022

potiuk commented Jun 30, 2022

eladkal commented Jul 3, 2022

		======================= ==================

		Cross provider package dependencies

Remove Tableau from Salesforce provider #23747

Remove Tableau from Salesforce provider #23747

Conversation

eladkal commented May 17, 2022

jhtimmins commented May 20, 2022

eladkal commented May 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashb left a comment

Choose a reason for hiding this comment

github-actions bot commented May 20, 2022

eladkal commented May 20, 2022

eladkal commented Jun 21, 2022

Choose a reason for hiding this comment

potiuk Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

eladkal Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk commented Jun 30, 2022

potiuk commented Jun 30, 2022

potiuk commented Jun 30, 2022

potiuk commented Jun 30, 2022

eladkal commented Jul 3, 2022

potiuk Jun 30, 2022 •

edited

Loading

potiuk Jun 30, 2022 •

edited

Loading

eladkal Jun 30, 2022 •

edited

Loading