Skip to content
This repository has been archived by the owner on Nov 14, 2019. It is now read-only.

Do not raise an error when a service is already started #131

Merged
merged 3 commits into from
Aug 22, 2019

Conversation

antho1404
Copy link
Member

@antho1404 antho1404 commented Aug 17, 2019

When a service is started and we execute the command service:start we have an error.

Now the command will catch the error, display a warning and return the hash based on the error.

@antho1404 antho1404 self-assigned this Aug 17, 2019
@antho1404 antho1404 added enhancement New feature or request ux labels Aug 17, 2019
@antho1404 antho1404 added this to the next milestone Aug 17, 2019
Copy link
Contributor

@krhubert krhubert left a comment

Choose a reason for hiding this comment

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

I would stay with an error in that case. Bash Scripts, ci tasks (any other tools/script/etc... for automation) might use the cli return code. Returning an error might significantly help debugging such scripts/tasks.

Also starting a service that has already been started should rather give an error (the same as deleting/stoping something that has been already deleted/stopped etc...).

@antho1404
Copy link
Member Author

Having these errors is really not developer-friendly. When developing a service we can have this error for example with the dev command, also the workflow that will create and start the service based on the hash or whatever. All this is just blocking the user. That's why I still kept the warning, it's important to know but it doesn't need to crash the task.

And for the part of a script using the cli then, in that case, the script will not crash and you will still have the instance of the service to continue your script. Having an error will sure help but having no error will just remove this problem and you will still be able to handle the real errors like the connection to the engine or stuff like that.

@krhubert
Copy link
Contributor

Having these errors is really not developer-friendly.

Why so? The errors tells the story and you can move on with developing.

All this is just blocking the user.

How this is blocking the user, i don't get it :(

Having an error will sure help but having no error will just remove this problem

On backend side, Script/Task automation doesn't work that way if you remove the error you won't have any pbm. It's rather opposite and my comment was exactly about that. On backend It's more like if you remove the error you will have more pbms. And backend/automation/etc perspectives should also be taken within each PRs. Still I don't see how the PR affects developers, but I see a lot of troubles with setting it up.

Copy link
Member

@NicolasMahe NicolasMahe left a comment

Choose a reason for hiding this comment

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

I agree with Hubert on this one.
The CLI should exit with an error and not just display a warning. (like the docker cli).
For the workflow resolver, the error should be catched and skipped.

}

export const resourceHash = (err: Error, resource: string): string => {
const reg = new RegExp(`${resource} \"(.*)\" already exists`)
Copy link
Member

Choose a reason for hiding this comment

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

This should be done using the gRPC Error code... This is not an small fix as the Engine needs to be updated. But using a regex is a hack.

Copy link
Member Author

Choose a reason for hiding this comment

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

definitely agree but unfortunately, for now, the engine doesn't return proper error code.

@antho1404
Copy link
Member Author

I did some updates, now the error is still triggered but typed and can be caught by the dev/compile commands

@antho1404 antho1404 removed the ux label Aug 21, 2019
@NicolasMahe NicolasMahe merged commit 3b2e96a into master Aug 22, 2019
@NicolasMahe NicolasMahe deleted the feature/start-error-handle branch August 22, 2019 07:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants