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

[mesheryctl] Merging PreRunE for command and Preflight checks #2779

Merged
merged 13 commits into from May 8, 2021

Conversation

piyushsingariya
Copy link
Contributor

Description
Read the full description #2767

This PR fixes #2767

Signed-off-by: Piyush Singariya <piyushsingariya@gmail.com>
Signed-off-by: Piyush Singariya <piyushsingariya@gmail.com>
Signed-off-by: Piyush Singariya <piyushsingariya@gmail.com>
@netlify
Copy link

netlify bot commented May 1, 2021

Deploy preview for meshery-docs ready!

Built with commit 077b484

https://deploy-preview-2779--meshery-docs.netlify.app

Signed-off-by: Piyush Singariya <piyushsingariya@gmail.com>
Signed-off-by: Piyush Singariya <piyushsingariya@gmail.com>
Signed-off-by: Piyush Singariya <piyushsingariya@gmail.com>
@pottekkat
Copy link
Contributor

@piyushsingariya Shouldn't every preReqCheck be changed?

@piyushsingariya
Copy link
Contributor Author

piyushsingariya commented May 2, 2021

Yeah, they should be changed but i didn't because preflight checks are still an experimental feature.
Should i change them?? Or we can make a few "good first issues" out of this.

@pottekkat
Copy link
Contributor

Yeah, they should be changed but i didn't because preflight checks are still an experimental feature.
Should i change them?? Or we can make a few "good first issues" out of this.

I think that it should be changed, unless we have any strong reason to do otherwise. The preflight checks have all the same functions as in the preRun function right? So unless we are missing anything, it would make sense to use the preflight function.

// @hexxdump @leecalcote

@piyushsingariya
Copy link
Contributor Author

piyushsingariya commented May 2, 2021

I think that it should be changed, unless we have any strong reason to do otherwise.

Waiting for a confirmation....

Signed-off-by: Piyush Singariya <piyushsingariya@gmail.com>
@hexxdump
Copy link
Contributor

hexxdump commented May 2, 2021

Yeah, they should be changed but i didn't because preflight checks are still an experimental feature.
Should i change them?? Or we can make a few "good first issues" out of this.

I think that it should be changed, unless we have any strong reason to do otherwise. The preflight checks have all the same functions as in the preRun function right? So unless we are missing anything, it would make sense to use the preflight function.

// @hexxdump @leecalcote

Agreed. However, not all commands call preRun function, wherever applicable, replace with preflight.

@leecalcote
Copy link
Member

@hexxdump +1

Signed-off-by: Piyush Singariya <piyushsingariya@gmail.com>
Signed-off-by: Piyush Singariya <piyushsingariya@gmail.com>
@piyushsingariya
Copy link
Contributor Author

@hexxdump @leecalcote @navendu-pottekkat I updated all the PreRunE with Preflight checks. 🥂

pottekkat
pottekkat previously approved these changes May 4, 2021
Copy link
Contributor

@pottekkat pottekkat left a comment

Choose a reason for hiding this comment

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

@piyushsingariya Looks good ⚡

@piyushsingariya piyushsingariya removed the request for review from metonymic-smokey May 6, 2021 14:08
@leecalcote
Copy link
Member

leecalcote commented May 6, 2021

@piyushsingariya, centralizing these check is fantastic. In terms of where they get exposed in the CLI, here's my thinking:

system check - checks on health of Meshery deployment (all components) in current context.
system check --adapter - checks on specific adapter; flag may be repeated.
system check --operator - verifies health of Meshery Operator deployment with MeshSync and Broker
system check --preflight - checks environmental prerequisites for deploying Meshery.
system check --report - runs diagnostics collection and bundles for opening an issue.

Given this, will you move preflight out of exp and move it from command to flag?

@piyushsingariya
Copy link
Contributor Author

piyushsingariya commented May 7, 2021

Given this, will you move preflight out of exp and move it from command to flag?

Yeah, I'll be removing the preflight command after this PR and will directly add the check command under exp and move preflight as a flag, and will start working on the other features under the same command.

@leecalcote
Copy link
Member

Sounds good, @piyushsingariya. Two tweaks:

  • you can move out of ‘exp’ now.
  • let’s chat about other areas of need before more efforts on check.

@piyushsingariya
Copy link
Contributor Author

piyushsingariya commented May 7, 2021

let’s chat about other areas of need before more efforts on check.

@leecalcote I'm up for it! 💪 ✌️

@leecalcote
Copy link
Member

@piyushsingariya here we are - #2823

@pottekkat
Copy link
Contributor

@piyushsingariya #2823 has your name written all over it!

Signed-off-by: Piyush Singariya <piyushsingariya@gmail.com>
Signed-off-by: Piyush Singariya <piyushsingariya@gmail.com>
Copy link
Member

@leecalcote leecalcote left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Member

@leecalcote leecalcote left a comment

Choose a reason for hiding this comment

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

Let's move forward and revisit as needed as we progress.

@leecalcote leecalcote merged commit 12d5ed8 into meshery:master May 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/mesheryctl CLI for Meshery kind/enhancement Improvement in current feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mesheryctl] Centralize preflight checks for meshery deployment and PreRunE for commands
4 participants