-
Notifications
You must be signed in to change notification settings - Fork 4
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
Health check on CLI on first start #256
Conversation
bae5260
to
3d4acbe
Compare
3d4acbe
to
cffac69
Compare
cffac69
to
d9ebf45
Compare
d9ebf45
to
496e809
Compare
496e809
to
52f71b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add option to remove colours and emojis as it causes issues sometimes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good. There is a single typo that Pat already pointed out
Otherwise, let's merge it.
Signed-off-by: Gabriel Bussolo <gabriel@calyptia.com>
8cf7cbe
to
7d8e47c
Compare
the workaround to keep both was too big just for this pr, so I decided to just remove the emojis and colors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo again
@vigneshcommits or @jasmingacic any good option for spell checking debug statements in Go?
7d8e47c
to
9019b34
Compare
|
I've never used any of those tools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm reasonably happy with this, it needs follow on updates as we add more checks.
Plus @gabrielbussolo can you raise an issue to update the calyptia/core-images vm/install-core.sh script to invoke it as well?
@jasmingacic please approve if you're happy to he can merge
func checkUrl(c *cfg.Config) bool { | ||
ctx := context.Background() | ||
_, err := c.Cloud.Environments(ctx, c.ProjectID, types.EnvironmentsParams{}) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This simple error check is not good enough. If the token is incorrect, or cloud is down for some reason, you will get the "internet connection issues" anyway. Which is not fair.
We should check if errors.Is(err, syscall.ECONNREFUSED)
to detect if the URL is wrong.
And hit the /healthz
endpoint to check if the server is up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure the cloud API endpoint is reachable with the given token (this can be performed with any simple get/listing operation). (issue#229)
The idea is literally to test if we can do a GET
with the given token, but I improved the message now including more causes of the connection not working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gabrielbussolo after this URL check just do quick connect to it with the token just to check validity of the token. You can do that in checkToken() fn.
I don't think that checkUrl is acutally going to help us at all because chances are that by doing calyptia create core_instance k8s
core will be installed in a remote cluster. What do you think?
@@ -75,6 +75,14 @@ func NewRootCmd(ctx context.Context) *cobra.Command { | |||
Short: "Calyptia Cloud CLI", | |||
SilenceErrors: true, | |||
SilenceUsage: true, | |||
PreRun: func(cmd *cobra.Command, args []string) { | |||
if _, err = config.LocalData.Get(cnfg.KeyCliHealth); errors.Is(err, localdata.ErrNotFound) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we store the health indicator, this code won't run ever again. And health is something that changes over time, or if the user passes different arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to the original issue, the idea it's run once after a fresh install, after this, if the client wants to check the status he should run the proper command for it.
After brew install or installing the binary directly, a quick set of checks has to be performed
in the local machine to ensure further operations will work (issue#229)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put it on the PreRun
was a suggestion but can be moved to the main func
again after the token and URL setup if you guys find it pertinent @jasmingacic @nicolasparada (i don't have a strong opinion about it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PreRun was something else. This is rather preflight check.
So don't worry about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments...
But I don't know what we are trying to accomplish here...
Having a global pre-run hook is an overkill, since it checks too many things. And have it to run only once, and then save that it's ok, and not run it ever again it's not good either 🤷♂️
Maybe reduce he scope and on the pre-run only check that that token is set, and /healthz
. Checking kubernetes might be too much, besides not all commands of the CLI require k8s. Maybe you just want to get some info about your pipelines, this will block you if you don't have access to the cluster.
9019b34
to
475588f
Compare
Signed-off-by: Gabriel Bussolo <gabriel@calyptia.com>
475588f
to
3112adb
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
@gabrielbussolo I'm going to park this for now I'm afraid, good work so far but I think we need to reconsider. |
Summary of this proposal
Depends on #300 to be aprovved
Notes for the reviewer
This runs on first execution of any CLI command.
We can also run it explicitly as
calyptia config check
.Issue(s) number(s)
Disclaimer: Please do not create a Pull Request without creating an issue first.
Checklist for submitter
Checklist for reviewer
Backport