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 BYOH cluster create sequence diagram #355

Merged
merged 1 commit into from
Feb 2, 2022
Merged

Add BYOH cluster create sequence diagram #355

merged 1 commit into from
Feb 2, 2022

Conversation

dharmjit
Copy link
Contributor

Co-authored-by: Anusha Hegde anushah@vmware.com

What this PR does / why we need it:

This PR adds a sequence diagram for BYOH cluster creation workflow.

Which issue(s) this PR fixes :
Fixes #

@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2022

Codecov Report

Merging #355 (e5d4629) into main (df97a2a) will not change coverage.
The diff coverage is n/a.

❗ Current head e5d4629 differs from pull request most recent head 35429ae. Consider uploading reports for the commit 35429ae to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main     #355   +/-   ##
=======================================
  Coverage   61.92%   61.92%           
=======================================
  Files          23       23           
  Lines        1731     1731           
=======================================
  Hits         1072     1072           
  Misses        587      587           
  Partials       72       72           

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 df97a2a...35429ae. Read the comment docs.

@huchen2021
Copy link
Contributor

How about adding "Patch BYOH Provider on Management Cluster"?

@dharmjit
Copy link
Contributor Author

How about adding "Patch BYOH Provider on Management Cluster"?

@huchen2021 , I do not have strong views on this, but my thoughts were that it's implied that the provider is installed when we say management cluster. I know, currently we have to patch BYOH provider separately but that might not be the case going forward.

@anusha94
Copy link
Contributor

How about adding "Patch BYOH Provider on Management Cluster"?

Yes, that's a valid point @huchen2021
We did discuss about this, but for cluster create, we are assuming the provider is already present on the management cluster - this is something as a preparation step and not really part of "Cluster Create" operation. Makes sense?

Co-authored-by: Anusha Hegde <anushah@vmware.com>
@pshail
Copy link
Contributor

pshail commented Jan 31, 2022

@dharmjit shall we create an arch.md file where we describe these diagrams and embed the png as well, wdyt?

Copy link
Contributor

@jamiemonserrate jamiemonserrate left a comment

Choose a reason for hiding this comment

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

LGTM!

@dharmjit
Copy link
Contributor Author

dharmjit commented Feb 1, 2022

@dharmjit shall we create an arch.md file where we describe these diagrams and embed the png as well, wdyt?

Yes, referring to these diagrams in other docs is a good idea for discovery.

@anusha94
Copy link
Contributor

anusha94 commented Feb 1, 2022

Yes, referring to these diagrams in other docs is a good idea for discovery.

Let's use #238 for this.

@pshail wdyt of the sequence diagram in this PR? Need changes? Good to merge?

@pshail
Copy link
Contributor

pshail commented Feb 2, 2022

overall /LGTM
@dharmjit minor nits of arrow types for return messages and wherever possible callout sync/async calls

@pshail
Copy link
Contributor

pshail commented Feb 2, 2022

@dharmjit let me take care of these nits in #238 , we can merge this
LGTM!

@dharmjit dharmjit merged commit 2fc856d into vmware-tanzu:main Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants