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

Add diagram of installer-based provisioning with subnet bootdisk #3309

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Lennonka
Copy link
Contributor

What changes are you introducing?

Adding a sequence diagram of installer-based provisioning with subnet bootdisk. Using PlantUML to build the diagram.

Why are you introducing these changes? (Explanation, links to references, issues, etc.)

Preparation for improvements of Provisioning hosts. The diagram will be used later to explain the workflow of the provisioning method.

Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)

  • Some files (.iuml, .pstyle) are expected to be reused in mutiple diagrams.
  • Puppet steps are not currenly displayed in the resulting image. Perhaps they should be for Foreman?
  • Continuation of Add diagrams of provisioning methods #3069

Checklists

  • I am okay with my commits getting squashed when you merge this PR.
  • I am familiar with the contributing guidelines.

Please cherry-pick my commits into: N/A

@Lennonka Lennonka added Needs tech review Requires a review from the technical perspective Needs style review Requires a review from docs style/grammar perspective labels Sep 20, 2024
Copy link

github-actions bot commented Sep 20, 2024

The PR preview for 20129f9 is available at theforeman-foreman-documentation-preview-pr-3309.surge.sh

The following output files are affected by this PR:

show diff

show diff as HTML

!if ($puppet)
opt when Puppet enabled
Host -> Puppet : sends CSR
Puppet -> Host : sends certificate (if allowed to)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would require an additional step -- a loop to check whether Puppet is allowed to send a certificate.

loop
Host -> Foreman : requests installation media
Foreman -> Proxy : requests installation media
Proxy -> Foreman : provides installation media
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the call would be "Installation media" because it is in response to a request. Same comment for next line.

@apinnick
Copy link
Contributor

Could you please embed the diagram in an adoc file so that we can see how the diagram renders?

!include prov-installation-media.iuml
note over Host : OS is installed
!include prov-initial-configuration.iuml
Host -> Foreman : calls home\n(disables build mode)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this mean?

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'm not entirely sure.

@stejskalleos Can you please explain what "call home" means? Could we use a different wording?

@Lennonka
Copy link
Contributor Author

Lennonka commented Oct 14, 2024

@apinnick There's a rendered PNG file of the diagram included with the PR for a preview.

Currently, there's no good place to include them.

@Lennonka
Copy link
Contributor Author

Lennonka commented Oct 14, 2024

@apinnick FYI, we're currently working on #3304, so I will start incorporating your suggestions there first. Some of the files (.iuml) are going to be reused in the other PlantUML diagrams once merged and rebased. Also, I think you mentioned that downstream expects sentence-case for the text in steps, so I'm going to apply that as well.

Thank you for your review, much appreciated!

@apinnick
Copy link
Contributor

@apinnick There's a rendered PNG file of the diagram included with the PR for a preview.

Currently, there's no good place to include them.

@Lennonka Seeing the rendered PNG file is not the same as seeing it in an .adoc file. If there is no good place to include this diagram, I recommend that you create a temporary .adoc file and embed it in an assembly (with lots of warnings that this is for review purposes only) so that we can see how a diagram of this size renders on the page. The text might be too small to read.

@Lennonka
Copy link
Contributor Author

Lennonka commented Nov 4, 2024

@stejskalleos GH won't let me assign you as a reviewer to this PR, so I'm tagging you :)

PS. I didn't manage to incorporate comments from previous PRs yet.

@Lennonka Lennonka force-pushed the provisioning-diagram-bootdisk-subnet branch from 2ff6914 to 20129f9 Compare November 6, 2024 20:30
@Lennonka
Copy link
Contributor Author

Lennonka commented Nov 6, 2024

Initial feedback incorporated. cc @stejskalleos

@Lennonka Lennonka marked this pull request as ready for review November 6, 2024 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs style review Requires a review from docs style/grammar perspective Needs tech review Requires a review from the technical perspective
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants