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

Mermaid example: onboarding sequence diagram #73

Open
wants to merge 2 commits into
base: pre-draft
Choose a base branch
from

Conversation

Silvanoc
Copy link
Contributor

Description

Replace a diagram added as an image with a diagram generated on-the-fly with Mermaid.

Issues Addressed

Diagrams as images are not editable and changes cannot be easily identified.

Change Type

Please select the relevant options:

  • Fix (change that resolves an issue)
  • New enhancement (change that adds specification content)
  • Content edits (change that edits existing content)

Checklist

  • I have read the CONTRIBUTING document.
  • My changes adhere to the established patterns, and best practices.

Modified 'workload-orchestration-edge-onboarding.md' to include a diagram example

Signed-off-by: Andreas Ziller <[email protected]>
Signed-off-by: Silvano Cirujano Cuesta <[email protected]>
@Silvanoc
Copy link
Contributor Author

This is as of now a PR meant to be used as discussion basis, therefore Draft. Some parts of it will be discussed directly in this PR. Micro-commits for easy understanding of the changes should be squashed before merging!

@g0zilla could you please have a look at the changes I've put onto yours?

@g0zilla
Copy link
Contributor

g0zilla commented Feb 10, 2025

@Silvanoc , this is okay from my side.

@Silvanoc
Copy link
Contributor Author

@Silvanoc , this is okay from my side.

I will then squash my micro-commits and mark the PR as ready for merging. Thanks for the quick feedback!

Improvements and fixes:

- Uniformize actor activation code in Mermaid sequence diagram.
- Fix erroneous arrows for return messages in onboarding sequence diagram.
- Small readibility improvement in onboarding sequence diagram: Request messages have a continuous line and returns a dashed line.
- Add to onboarding sequence diagram a box showing the proposal for using a FIDO onboarding workflow.
- Improve readability
- Add legend
- Document possible sequence diagram change
- Typos

Signed-off-by: Silvano Cirujano Cuesta <[email protected]>
@Silvanoc Silvanoc force-pushed the mermaid-onboarding-sequence branch from 956b3ad to 89c3a3f Compare February 10, 2025 16:10
@Silvanoc Silvanoc marked this pull request as ready for review February 10, 2025 16:11
@Silvanoc Silvanoc requested a review from a team as a code owner February 10, 2025 16:11
@ajcraig ajcraig self-requested a review February 12, 2025 21:19
Copy link
Contributor

@ajcraig ajcraig left a comment

Choose a reason for hiding this comment

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

This workflow looks good and demonstrates how Fido on boarding could assist us in on boarding the API client to the Server within the Workload Fleet Manager.

  • There is a note that FIDO has not been selected, but I it's positive to include as a proposal within the documentation. This is still a decision we need to finalize within the Management interface working group.

As for the content, I would recommend removing the sections in the mermaid drawing beyond line # 57. I mention this as I think we should have a sequence diagram within the workload deployment markdown describing this process. I have a draft of this sequence in the following issue comment

Additionally, lines 55-71 mention the sequence for Git storage and retrieval. This strategy to utilize or mandate git storage was not approved by the TWG. I think keeping line 55 shows a crucial step in onboarding the client, however, we should make a note that it is still under discussion how the client will receive url and any access related tokens/secrets to the location where the desired states will be stored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants