Skip to content

Conversation

@runcom
Copy link
Member

@runcom runcom commented Apr 8, 2015

@LK4D4

Signed-off-by: Antonio Murdaca me@runcom.ninja

Copy link
Contributor

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?

Copy link
Member Author

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

Copy link
Member Author

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)

Copy link
Contributor

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

@runcom runcom force-pushed the remove-job-rm branch 2 times, most recently from 9ba9191 to 771ae35 Compare April 8, 2015 22:18
daemon/delete.go Outdated
Copy link
Member Author

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)

Copy link
Contributor

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.

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 8, 2015

@runcom Cool, thanks.

@runcom
Copy link
Member Author

runcom commented Apr 8, 2015

my pleasure

Copy link
Contributor

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?

Copy link
Contributor

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 :)

Copy link
Member Author

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...

Copy link
Contributor

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

Copy link
Member

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

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.

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 8, 2015

@runcom Integration tests failed.

@runcom
Copy link
Member Author

runcom commented Apr 8, 2015

@LK4D4 fixing

daemon/delete.go Outdated
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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

@runcom runcom force-pushed the remove-job-rm branch 2 times, most recently from 965ae3f to 4aa01b9 Compare April 9, 2015 09:31
@runcom runcom mentioned this pull request Apr 9, 2015
@runcom
Copy link
Member Author

runcom commented Apr 9, 2015

@cpuguy83 I've introduced a config struct as you suggested

(sorry I wrongly closed the pull request)

@runcom runcom closed this Apr 9, 2015
@runcom runcom reopened this Apr 9, 2015
@LK4D4
Copy link
Contributor

LK4D4 commented Apr 9, 2015

@runcom Could you rebase against master pls?

@LK4D4
Copy link
Contributor

LK4D4 commented Apr 9, 2015

@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>
@runcom
Copy link
Member Author

runcom commented Apr 9, 2015

rebased and removed that test, that scenario is well tested in integration-cli @LK4D4

@cpuguy83
Copy link
Member

cpuguy83 commented Apr 9, 2015

LGTM

1 similar comment
@LK4D4
Copy link
Contributor

LK4D4 commented Apr 9, 2015

LGTM

LK4D4 added a commit that referenced this pull request Apr 9, 2015
@LK4D4 LK4D4 merged commit 3218f39 into moby:master Apr 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants