Skip to content

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Aug 18, 2020

Part of #10368


^ 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 UPDATING.md.

@potiuk
Copy link
Member Author

potiuk commented Aug 18, 2020

This is purely a documentation change describeing the future CI architecture as well adding mermaid support for generation of sequence diagrams so that it is easier to review.

@potiuk potiuk force-pushed the future-ci-documentation branch 4 times, most recently from 0497089 to 4139b63 Compare August 19, 2020 00:07
@potiuk potiuk force-pushed the future-ci-documentation branch 2 times, most recently from 44bb1ae to 4ccb131 Compare August 19, 2020 18:13
@potiuk potiuk changed the title Added Documentation for future CI with mermaid sequence diagrams Updated Documentation for the CI with mermaid sequence diagrams Aug 19, 2020
@potiuk
Copy link
Member Author

potiuk commented Aug 19, 2020

Ad discussed in #10368 - I am going to merge that one after the code is reviewed/fixed/merged

@potiuk potiuk force-pushed the future-ci-documentation branch 3 times, most recently from 324dccb to 7a328d2 Compare August 20, 2020 07:58
@potiuk
Copy link
Member Author

potiuk commented Aug 20, 2020

Rebased after merging #10368 - I think it is ready for review and very accurately describes what we have in CI now.

@potiuk
Copy link
Member Author

potiuk commented Aug 20, 2020

Would love to merge the docs to close the task!

@potiuk potiuk force-pushed the future-ci-documentation branch from 7a328d2 to a40f9e9 Compare August 21, 2020 13:50
@potiuk
Copy link
Member Author

potiuk commented Aug 21, 2020

Udpated the first review by @feluelle .

@potiuk potiuk force-pushed the future-ci-documentation branch from a40f9e9 to f7151f3 Compare August 22, 2020 14:12
@potiuk
Copy link
Member Author

potiuk commented Aug 22, 2020

Latest additions and rebasing to latest master. I would love to get this one reviewed - it's purely documentation of the CI setup we have now, - but it includes mermaid-generated images, and I believe @kaxil and @mik-laj have some doubts about it, so I would love to hear more about it :)

@potiuk
Copy link
Member Author

potiuk commented Aug 24, 2020

https://drawio-app.com/create-mermaid-diagrams-in-draw-io/ -> There is an integration that is already available

So you basically say you want to still keep the sources of mermaid but rather than doing git commit and have them automatically generated you want me to always go to draw,io, copy&paste the code, generate the image, download them, replace the images, commit (and not forget to copy back the sources if I changed them in draw,io?)

Because that's the alternative. Currently the only thing you need to do is to commit the sources and the images will be generated autopmatically.

This is like forcing me to do those manual steps where simple (literally 20 lines of integration code that will never change) can do it for me. I do not really see the logic behind it.

@potiuk
Copy link
Member Author

potiuk commented Aug 24, 2020

I already plan some changes in those diragras - for example when I implement #10507 - there will be quite some changes there and simply changing that in text is so much easier than having to do all those manual steps.

@kaxil
Copy link
Member

kaxil commented Aug 24, 2020

I am definitely against have a CI-CD for a single tool, which if it breaks for whatever reason is a code debt we should not have.

These can be easily replaced by a generated PNGs.

@kaxil
Copy link
Member

kaxil commented Aug 24, 2020

You don't see the logic behind it because you are not reasoning for the amount of code that needs to be maintained

@potiuk
Copy link
Member Author

potiuk commented Aug 24, 2020

I am definitely against have a CI-CD for a single tool, which if it breaks for whatever reason is a code debt we should not have.

It only runs in CI-CD if the source mermaid files change. We keep hashes of the files and no mermaid will be run when then .mermaid files are not touched.

@kaxil
Copy link
Member

kaxil commented Aug 24, 2020

Maintaining a separate node file just for mermaid makes 0 sense

@potiuk
Copy link
Member Author

potiuk commented Aug 24, 2020

Maintaining a separate node file just for mermaid makes 0 sense

If package.json is the problem, then no worries. I can remove the package.json and move it to the script generating mermaid (which will only be run when .mermaid files change). Woudl that work?

@kaxil
Copy link
Member

kaxil commented Aug 24, 2020

Are the package.json and the files for building mermaid diagrams in some other PR? I remember seeing them

@potiuk
Copy link
Member Author

potiuk commented Aug 24, 2020

Are the package.json and the files for building mermaid diagrams in some other PR? I remember seeing them

Just looking at that. I think they are lost on rebase :)

@potiuk
Copy link
Member Author

potiuk commented Aug 24, 2020

Are the package.json and the files for building mermaid diagrams in some other PR? I remember seeing them

Just looking at that. I think they are lost on rebase :)

I will restore them in the way I think they can work without package.json.

@potiuk potiuk force-pushed the future-ci-documentation branch from 4f82687 to 9861efc Compare August 24, 2020 17:04
@potiuk
Copy link
Member Author

potiuk commented Aug 24, 2020

It's there. Standalone bash pre-commit script that will only do anything if any of the .mermaid files changed vs. hashes stored next to them.

That "anything" is installing mermaid in a separate node virtualenv (in ,build/mermaid folder - the ".build" folder is already all but ignored due to being used by breeze). It will install mermaid there, first time when it is run and keep on reusing it every next time it is run.

If there is no change in those files or if you have no npm installed, the pre-commit will silently succeed. If you do not touch any of those files, it's a no-op - both for the developers and for the CI. First of all pre-commit will be skipped if those files are not changed, secondly - even if you run pre-commit with --all-files those files will be skipped as the .md5 hashes will not change for them. And even if they do, but you have no npm, they will be silently ignored.

I even made sure not to commit the mermaid-config.json - it is dynamically generated by the self-contained bash script. I expect this script to never change (unless we want to update colors/fonts etc. in the generated mermaid files).

@kaxil
Copy link
Member

kaxil commented Aug 24, 2020

CI is failing, maybe needs a rebase

@potiuk potiuk force-pushed the future-ci-documentation branch from 9861efc to 1ef6fbb Compare August 24, 2020 20:06
@potiuk potiuk merged commit f2da6b4 into apache:master Aug 24, 2020
@potiuk potiuk deleted the future-ci-documentation branch August 24, 2020 20:45
potiuk added a commit that referenced this pull request Nov 14, 2020
@potiuk potiuk added this to the Airflow 1.10.13 milestone Nov 14, 2020
@potiuk potiuk added the type:misc/internal Changelog: Misc changes that should appear in change log label Nov 14, 2020
potiuk added a commit that referenced this pull request Nov 16, 2020
potiuk added a commit that referenced this pull request Nov 16, 2020
kaxil pushed a commit that referenced this pull request Nov 18, 2020
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools type:misc/internal Changelog: Misc changes that should appear in change log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants