Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Dockerized docs gen #295

Merged
merged 3 commits into from
May 24, 2022
Merged

Conversation

MorpheusXAUT
Copy link
Contributor

TL;DR

Docs generation currently still relies on locally installed versions of protoc, has now been changed to use Docker containers as well.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

The current setup for auto-generating documentation (within generate_protos.sh) still relies on a local version of protoc being installed, which is not managed/set up by boilerplate or repo dependencies either, requiring a new dev to set up the (correct) version locally themselves.
In order to allow for easier (and more consistent) docs generation, these calls have been changed to use the official pseudomuto/protoc-gen-doc as well, no longer requiring any local dependencies for the generate_protos.sh script.

The image used includes protoc-gen-doc version 1.4.1, which the flyte fork is built on. Whilst the flyte fork adds "native" support for reStructuredText templates, hardly any of the documentation generated uses the new type/template - except core, all other packages are generated using a custom template within the flyteidl repository anyways. I have thus decided to simply move the template used for core to the repository and keep it with the other, already existing one.
The fork's restructuredtext renderer was using protoc-gen-doc's HTMLRenderer, all other custom templates default to the TextRenderer though. I believe this change should be irrelevant for this documented generation though as both templates don't seem to be using HTML exclusive functionality anyways (except HTML comment blocks, which I have replaced with the equivalent rst ones).
One change that is relevant though is the use of double quotes in proto field comments: the HTMLRenderer URL-escapes those quotes while the TextRenderer doesn't, leading to incorrectly generated docs with missing field tables. This can be circumvented by using backticks or single quotes though - all existing " in comments have been replaced with ' in the core protos.
Furthermore, the pseudomuto/protoc-gen-doc:1.4.1 Docker image unfortunately only uses protoc version 3.6.1 (instead of 3.20.1 as used in the current GH workflow). Whilst the code generated seems to be identical (and only affects docs anyways), I'm not sure if that's a dealbreaker for this PR. We could bump the used image up to pseudomuto/protoc-gen-doc:1.5.0 (the last one seems to be broken somehow), however we'd still be stuck with protoc 3.6.1...

Last but not least, I've set a locale override to C.UTF-8 in the generate_protos.sh script. This ensures that the sorting of the generated documentation is consistent between all systems (a different locale was the reason for the ordering issues I had during my last PR).

Tracking Issue

fixes flyteorg/flyte#2437

Follow-up issue

NA

Nick Müller added 3 commits May 3, 2022 11:18
Now uses protoc-gen-dec Docker image instead of running protoc using the protoc-gen-doc plugin locally

Signed-off-by: Nick Müller <[email protected]>
Replaced double with single quotes in proto comments (cause escaping issues with protoc-gen-doc text renderer)

Signed-off-by: Nick Müller <[email protected]>
@codecov
Copy link

codecov bot commented May 3, 2022

Codecov Report

Merging #295 (0b1038c) into master (e749b82) will not change coverage.
The diff coverage is n/a.

❗ Current head 0b1038c differs from pull request most recent head db59fcf. Consider uploading reports for the commit db59fcf to get more accurate results

@@           Coverage Diff           @@
##           master     #295   +/-   ##
=======================================
  Coverage   74.77%   74.77%           
=======================================
  Files          15       15           
  Lines         991      991           
=======================================
  Hits          741      741           
  Misses        218      218           
  Partials       32       32           
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e749b82...db59fcf. Read the comment docs.

Copy link
Contributor

@katrogan katrogan left a comment

Choose a reason for hiding this comment

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

this is awesome! thank you for implementing this and fixing the issues with quotes

generate_protos.sh Show resolved Hide resolved
@katrogan
Copy link
Contributor

katrogan commented May 3, 2022

PTAL @cosmicBboy @EngHabu

@MorpheusXAUT
Copy link
Contributor Author

Anything else needed to get this merged?
As we're currently planning our next Flyte changes that might require IDL adaptions, it'd be great to be able to use this instead of having to run docs generation on a different machine 😅

@EngHabu EngHabu merged commit a7b46a2 into flyteorg:master May 24, 2022
@MorpheusXAUT MorpheusXAUT deleted the dockerized-docs-gen branch May 24, 2022 16:41
@kumare3
Copy link
Contributor

kumare3 commented May 24, 2022

@MorpheusXAUT and gang, also we should start planning for using buf build at some point

eapolinario pushed a commit that referenced this pull request Sep 8, 2023
* Dockerized docs generation
Now uses protoc-gen-dec Docker image instead of running protoc using the protoc-gen-doc plugin locally

Signed-off-by: Nick Müller <[email protected]>

* Minor cleanup of doc templates
Replaced double with single quotes in proto comments (cause escaping issues with protoc-gen-doc text renderer)

Signed-off-by: Nick Müller <[email protected]>

* Set locale override during protos/docs generation to ensure consistent sorting

Signed-off-by: Nick Müller <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Docs] Dockerize flyteidl docs generation
5 participants