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

✨ Update docs/Makefile #7033

Merged
merged 1 commit into from
Aug 10, 2022
Merged

Conversation

oscr
Copy link
Contributor

@oscr oscr commented Aug 6, 2022

What this PR does / why we need it:

I noticed that the docs/Makefile diagrams target uses an old PlantUML container from k8s.gcr.io. I took some inspiration from OpenStack provider and updated the Makefile in the following way.

  • Add a help target.
  • Add diagram build container target.
  • Update diagram build target to use local image.
  • Update Dockerfile usage instructions.

Finally I also regenerated the proposal diagrams since they where generated with an older version of PlantUML.

Makefile help output:

make 

Usage:
  make <target>

build
  diagrams         Make PlantUML diagrams
  plantuml-builder  Make diagram build container

general
  help             Display this help

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 6, 2022
@k8s-ci-robot
Copy link
Contributor

@oscr: This issue is currently awaiting triage.

If CAPI contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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 the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 6, 2022
@oscr
Copy link
Contributor Author

oscr commented Aug 6, 2022

/test pull-cluster-api-verify-main

@oscr oscr force-pushed the fix-docs-makefile branch 2 times, most recently from 5cc52ee to 7ac2411 Compare August 6, 2022 21:43
Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

I think we should be careful with the differences between the new and old images - some of them appear to be incorrectly formatted so we might need to update the plantuml files so that the resulting images are closer to the originals.

Would it be possible to do this incrementally? I understand there might be a large amount of work to update the images so the newer version of plantuml produces something close to the original versions. If it's possible we could create an umbrella task to update these images one-by-one (or batch by batch)

@oscr
Copy link
Contributor Author

oscr commented Aug 8, 2022

What would you say about removing the updates to proposals and change the "make diagrams" just builds the CAPI book? The proposals don't need to be updated so we can just let them be. Then in a separate pr we can either update the diagrams target or add a separate target for proposals diagrams together with fixes for them. Sounds ok @killianmuldoon?

@killianmuldoon
Copy link
Contributor

Could we maybe add two targets - make diagrams-book and make diagrams-proposals? We could then aggregate them under make-diagrams. I don't think we run this command as part of any wider workflow so it would be fine to exclude the proposals for now.

We would need a solution to the broken proposal images sooner rather than later though as it would impact anyone trying to create new images for a proposal.

@killianmuldoon
Copy link
Contributor

killianmuldoon commented Aug 9, 2022

I must have missed the updates on #7017 - it looks like there's a few issues there as well. Have you made any progress on figuring out why the layering / opacity is changing? I think that's the fundamental problem, everybody else seemed happy with the colors on that PR 😆

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 10, 2022
@killianmuldoon
Copy link
Contributor

Thanks for making the updates!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 10, 2022
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2022
@k8s-ci-robot k8s-ci-robot merged commit 5d92925 into kubernetes-sigs:main Aug 10, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.3 milestone Aug 10, 2022
@oscr oscr deleted the fix-docs-makefile branch August 10, 2022 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants