Skip to content

Conversation

@leahecole
Copy link
Contributor

After discovering this the hard way, I wanted to update the docs based on my slack convo with @potiuk and @mik-laj

@leahecole leahecole requested review from mik-laj and potiuk November 17, 2020 20:27
@boring-cyborg boring-cyborg bot added the provider:google Google (including GCP) related issues label Nov 17, 2020
@potiuk
Copy link
Member

potiuk commented Nov 17, 2020

The readme is automatically generated based on template: https://github.com/apache/airflow/blob/master/dev/provider_packages/PROVIDER_README_TEMPLATE.md.jinja2. And it will be overwritten the next time we generate it (which is most likely tomorrow).

It's next to impossible to maintain those READMEs separately for 60 providers we have so we always generate them before releasing new set of packages.

However, this template has one place where you can add per-provider specific information that will be added automatically to all the future READMEs - > {{ ADDITIONAL_INFO }}.

It is taken from ADDITIONAL_INFO.md file that should be added in the appropriate provider folder (in this case "airflow/providers/google/ADDITIONAL_INFO.md"). So this explanation should be added there in order to include it next time were generate the README.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

As explaine - it should be added in ADDITIONAL_INFO.md file

@leahecole
Copy link
Contributor Author

Excellent - makes sense, I'll make that change!

@leahecole
Copy link
Contributor Author

plz enjoy the comic relief of me mistakenly adding and removing blank lines all over the place as evidenced by my commits 😆

@leahecole leahecole requested a review from potiuk November 17, 2020 22:27
@leahecole
Copy link
Contributor Author

leahecole commented Nov 17, 2020

CI seems like it's failing because of lack of license header - I was poking around and it seems like the jinja templates have headers in them, but the generated .md files here do not. Should this new ADDITIONAL_INFO.md comply with the demands of the linter, or should it match with the generated ones?

@potiuk
Copy link
Member

potiuk commented Nov 18, 2020

CI seems like it's failing because of lack of license header - I was poking around and it seems like the jinja templates have headers in them, but the generated .md files here do not. Should this new ADDITIONAL_INFO.md comply with the demands of the linter, or should it match with the generated ones?

It should contain the licence.

The licence comment gets removed when the content is read:

if line.startswith(" -->"):

And you can see other ADDITIONAL_INFO.md files we have (this is the kubernetes one):

https://raw.githubusercontent.com/apache/airflow/master/airflow/providers/cncf/kubernetes/ADDITIONAL_INFO.md

The rule of thumb is that if a file is generated from other sources, even if it is committed, it does not have to have a licence. It's not per-se documented in ASF rules but this the information that we get when we run the official RAT tool from Apache:
Generated files do not require license headers

*****************************************************
Summary
-------
Generated at: 2020-11-18T07:51:01+01:00

Notes: 0
Binaries: 0
Archives: 0
Standards: 990

Apache Licensed: 990
Generated Documents: 0

JavaDocs are generated, thus a license header is optional.
Generated files do not require license headers.

0 Unknown Licenses


## Additional limitations

The [2020.11.13 release](#release-20201113) of this provider is only usable with Apache Airflow >= 1.10.12 version due to refactorings implemented in
Copy link
Member

@potiuk potiuk Nov 18, 2020

Choose a reason for hiding this comment

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

One comment here @leahecole -> I think it's not precise.

The package, in general, should be usable and installable with <1.10.12 - only the GKEPodOperator has some problems with <1.10.12. This is because cncf.kubernetes provider cannot be installed on airflow < 1.10.11 (and there were some known issues with cncf.kubernetes + 1.10.11). The cncf.kubernetes backport provider is an optional cross-dependency for the google backport provider). So we have actually not limited the google package to >=1.10.12 - you can still install it with airflow 1.10.9 and it will continue to work (except the GKEPodOperator)

Copy link
Member

Choose a reason for hiding this comment

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

At least this is what we think it is. As far as I remember, you mentioned there are problems with installing the provider in composer environment with 1.10.9 so I am wondering where it came from in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had problems installing it in a Composer environment with 1.10.10, (1.10.11 isn't offered in Composer) but not 1.10.12, @mik-laj mentioned that the latest version of this package was only compatible with the latest version of Composer but earlier version of the package are compatible with older versions of Airflow - I'm happy to have you adjust the wording as you see fit - I'm just trying to emulate the language used with the kubernetes package and the info given to me by Kamil and get it documented centrally so no one else has to figure this out the hard way 😄

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@potiuk
Copy link
Member

potiuk commented Nov 30, 2020

Hey @leahecole -> sorry for not getting back on it but we are super-busy with preparing for 2.0.0 (and we have out-of-band 1.10.14 that I am working on getting a consistent set of dependencies). Will come back on it next week!

@leahecole
Copy link
Contributor Author

No prob - I was offline for a few days due to a US holiday. If you want to set up a time to synchronously chat about this communication please let me know, and then if we do that, I can update the comments with our discussion details for transparency :)

@leahecole leahecole closed this Jan 19, 2021
@leahecole leahecole deleted the update_backport_doc branch January 19, 2021 23:36
@potiuk
Copy link
Member

potiuk commented Jan 20, 2021

Ah yeah. Sorry for that @leahecole It was there waiting for some "freeier" time but it never came :(

@leahecole
Copy link
Contributor Author

Ha! No worries. I did it mainly to clean up my PRs because I'm not focused on this right now. As the backport providers mature, I think things will be come more clear and this won't be as much of an issue.

@potiuk
Copy link
Member

potiuk commented Jan 20, 2021

I think backports are already kinda "on their way out" - they have 2 months EOL (end of March) and we are not going to release them any more after that date. I am more focusing on the regular providers of 2.0:

Maybe you can take a look instead at the new providers docs which we are discussing in #13767 (comment) and specifically take a look at the proposal on how the new "provider" documentation will look here: http://gabby-cough.surge.sh/. It would be great to get some comments from someone who might - sooner or later - want to use the providers!

Any comments there are most welcome @leahecole !

@leahecole
Copy link
Contributor Author

I'll take a look at it either later today or tomorrow!

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

Labels

AIP-8 - Provider packages provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants