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

Log drains: delete command #567

Merged
merged 7 commits into from May 25, 2020
Merged

Conversation

curzolapierre
Copy link
Contributor

@curzolapierre curzolapierre commented May 25, 2020

Add remove command

Example:

$ scalingo log-drains-remove --app sample-go-martini tcp+tls://logs.papertrailapp.com:12345
-----> The log drain: tcp+tls://logs.papertrailapp.com:12345 has been deleted

Fix #554

@Soulou
Copy link
Member

Soulou commented May 25, 2020

Why does the user need to use an URL? Didn't we agree, it should be "easier", using option flags etc.?

Copy link
Member

@Soulou Soulou left a comment

Comment above ^

@curzolapierre
Copy link
Contributor Author

curzolapierre commented May 25, 2020

@Soulou We planned to use uuid for the MVP. But as we are only on the first step of the MVP here, without uuid implemented yet, we (with @EtienneM) decided to use urls returned by the list command.
I agree on the point it is less consistent to use url, but flags are usefull to control url generation and are less relevant in case of deletion.
What do you think ?

@Soulou
Copy link
Member

Soulou commented May 25, 2020

So if I understand this well, the problem is that we don't have a unique identifier for each log drain, so we can't ask users to use them for the deletion, right?

@curzolapierre
Copy link
Contributor Author

curzolapierre commented May 25, 2020

I hadn't thought about that, the logs service will allow to add multiple same url by application ?
The current implementation of addition on the API will return an error if the user try to add an existing url.

Copy link
Member

@EtienneM EtienneM left a comment

Please also update the changelog

cmd/autocomplete/log_drains_remove.go Show resolved Hide resolved
cmd/autocomplete/log_drains_remove.go Outdated Show resolved Hide resolved
cmd/log_drains.go Outdated Show resolved Hide resolved
log_drains/remove.go Outdated Show resolved Hide resolved
log_drains/remove.go Outdated Show resolved Hide resolved
@curzolapierre curzolapierre force-pushed the feature/554/add_log_drains_del_cmd branch from c0a3a32 to a668076 Compare May 25, 2020
@curzolapierre curzolapierre requested a review from EtienneM May 25, 2020
Copy link
Member

@EtienneM EtienneM left a comment

LGTM. Just waiting for Scalingo/go-scalingo#167 to be merged and released

Copy link
Member

@EtienneM EtienneM left a comment

I released a new version of go-scalingo (4.5.3). Please sync with it and LGTM :)

Gopkg.toml Outdated Show resolved Hide resolved
@curzolapierre curzolapierre requested a review from EtienneM May 25, 2020
Gopkg.lock Show resolved Hide resolved
@curzolapierre curzolapierre force-pushed the feature/554/add_log_drains_del_cmd branch from 224e1b9 to 2d8474b Compare May 25, 2020
@EtienneM EtienneM merged commit b50f48a into master May 25, 2020
1 check passed
@EtienneM EtienneM deleted the feature/554/add_log_drains_del_cmd branch May 25, 2020
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.

3 participants