Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure reproducible image generation for docs and proposals #9331

Closed
killianmuldoon opened this issue Aug 29, 2023 · 8 comments · Fixed by #9363
Closed

Ensure reproducible image generation for docs and proposals #9331

killianmuldoon opened this issue Aug 29, 2023 · 8 comments · Fixed by #9363
Assignees
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@killianmuldoon
Copy link
Contributor

CAPI uses plantuml to generate images used in the CAPI book and CAEPs. A number of the images in the repo do not have plantuml files for generation.

To improve the situation we should create a verify make target which verifies we are able to regenerate images as expected and catches changes in the images after generation. These changes can happen after changing, for example, the plantuml verison used and should be caught and closely reviewed before merging.

As part of this we should decide if all of the images which currently have no plantuml source should be kept, or if it's possible to replace them. Those images that are retained should be retained in their current format and excepted from reproducible image building.

At time of writing the diff after deleting all pngsand regenerating looks like the below. Additionally the svg images in the repo are not currently generated using plantuml automatically.

        deleted:    docs/book/src/images/describe-cluster-disable-grouping.png
        deleted:    docs/book/src/images/describe-cluster-echo.png
        deleted:    docs/book/src/images/describe-cluster-how-grouping-works.png
        deleted:    docs/book/src/images/describe-cluster-show-conditions.png
        deleted:    docs/book/src/images/describe-cluster.png
        deleted:    docs/book/src/images/introduction.png
        deleted:    docs/book/src/images/runtime-sdk-lifecycle-hooks.png
        deleted:    docs/proposals/images/capi-provider-operator/fig3.png
        deleted:    docs/proposals/images/capi-provider-operator/fig4.png
        deleted:    docs/proposals/images/clusterctl-redesign/components.png
        deleted:    docs/proposals/images/conditions/cluster-provision-workflow.png
        deleted:    docs/proposals/images/conditions/upgrade-workflow.png
        deleted:    docs/proposals/images/externally-managed-cluster-infrastructure/infrastructure.png
        deleted:    docs/proposals/images/in-place-propagation/current-state.png
        deleted:    docs/proposals/images/in-place-propagation/optional-changes.png
        deleted:    docs/proposals/images/in-place-propagation/proposed-changes.png
        deleted:    docs/proposals/images/ipam-integration/consumption.png
        deleted:    docs/proposals/images/ipam-integration/sequence.png
        deleted:    docs/proposals/images/machine-states-preboot/Figure2.png
        deleted:    docs/proposals/images/runtime-hooks/runtime-hooks.png
        deleted:    docs/proposals/images/runtime-sdk/overview.png
        deleted:    docs/proposals/images/runtime-sdk/swagger-ui.png
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Aug 29, 2023
@killianmuldoon
Copy link
Contributor Author

/help
/good-first-issue
/triage accepted

@k8s-ci-robot
Copy link
Contributor

@killianmuldoon:
This request has been marked as suitable for new contributors.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/help
/good-first-issue
/triage accepted

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 29, 2023
@sbueringer
Copy link
Member

sbueringer commented Aug 29, 2023

Should we make it two sub-tasks to: 1) run the verification in CI 2) figure out what to do about png's without plantuml/ any other sources?

I think we don't necessarily have to couple it (I don't see a quick resolution to figure out what to do with all non-plantuml pngs)

I think it's also a totally separate discussion if we want to force everything onto plantuml (vs allowing other sources like drawio, excalidraw ... not sure, but we might have that today)

@killianmuldoon
Copy link
Contributor Author

I think we should couple it as figuring out which images we should compare is essentially the second part of the task. Without excluding those images I'm not sure how we can properly verify image generation in the CI, but open to ideas.

@sbueringer
Copy link
Member

I think we can re-generate plantuml without having to delete all pngs. It's already a win.

@typeid
Copy link
Contributor

typeid commented Aug 31, 2023

So is the task here to just add the plantuml generation to CI, to ensure that the generation works? I'd be interested in taking this on if that's the case.
If not, I'd still be interested but would need more clarification :)

@killianmuldoon
Copy link
Contributor Author

Thanks @typeid - I think the first task - as @sbueringer outlined - would be to have a verify-diagrams make target where we regenerate the images and ensure nothing has changed.

@typeid
Copy link
Contributor

typeid commented Sep 1, 2023

/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants