-
Notifications
You must be signed in to change notification settings - Fork 18.9k
Remove job from rm #12197
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
Remove job from rm #12197
Conversation
api/server/server.go
Outdated
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.
Seems like changing behavior and not consistent with other stuff.
Maybe just forceRemove := r.Form.Get("force") != ""
and remove vars?
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.
mmm why changing behaviour? jobs arguments were parsed to bool. Ok to remove vars btw
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 wanted to directly pass bools to ContainerRm instead of strings to be converted (as job was doing if i'm right)
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 don't see where they was parsed at all. only r.Form.Get
9ba9191 to
771ae35
Compare
daemon/delete.go
Outdated
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.
@LK4D4 this basically do the same as https://github.com/docker/docker/blob/master/engine/env.go#L57
but that one use env to get the key etc etc...I don't like this toBool here... should I move it elsewhere? (where)
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.
Better to be in server. This doing basically the same that was jobs.
|
@runcom Cool, thanks. |
|
my pleasure |
api/server/server.go
Outdated
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 know this function is Env.GetBool, but god it's horrible... :-(
It's fine to keep it as it was before, but with all the recent PR to remove engine.Job, hasn't anyone already implemented that one somewhere else?
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.
Nope, so probably somewhere something broken :)
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.
mmm haven't reallly looked at that. But I firstly thought about moving this to pkg/stringutils but it's not so generic to convert a string to a bool that way so I skipped that...then as lk4d4 said I moved it here so if someone other needs it (and she will because many bool flags are passed to daemon that way) she may reuse it.. I think this is the new implementation of Env.GetBool that server should use to convert string to bool maintaining backward compatibility... to my knowledge of course...
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.
what about decoding the Form into a struct instead wouldn't that solve this?
EDIT: ugh url params sorry
Also http://www.gorillatoolkit.org/pkg/schema
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.
Nice!
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.
Problem that we need to preserve weird old behavior.
|
@runcom Integration tests failed. |
|
@LK4D4 fixing |
daemon/delete.go
Outdated
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 might be a good candidate for a struct with so many bool args.
wdyt?
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 ok with it, I've followed your discussion on irc about 5+ args then struct so I thought it would make sense not to do a struct.
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.
Yeah, the issue is you end up calling this function like this daemon.ContainerRm("foo", true, true, false), and it's difficult to tell what the bools are really for without digging.
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.
@cpuguy83 alright, I'll make a struct for this then
965ae3f to
4aa01b9
Compare
|
@cpuguy83 I've introduced a config struct as you suggested (sorry I wrongly closed the pull request) |
|
@runcom Could you rebase against master pls? |
|
@runcom You forgot to fix integration test btw. Maybe it'll be easier to remove it or move to integration-cli? |
Signed-off-by: Antonio Murdaca <me@runcom.ninja>
|
rebased and removed that test, that scenario is well tested in integration-cli @LK4D4 |
|
LGTM |
1 similar comment
|
LGTM |
@LK4D4
Signed-off-by: Antonio Murdaca me@runcom.ninja