search

deis / builder

Git server and application builder for Deis Workflow
https://deis.com
MIT License
40 stars 41 forks source link

fix(pkg/types.go): change Dockerfile field to a bool #276

Closed arschles closed 8 years ago

arschles commented 8 years ago

Fixes https://github.com/deis/builder/issues/96 Requires https://github.com/deis/controller/issues/278

cc/ @helgi

arschles commented 8 years ago

@helgi should I still pursue this change? Not sure what the controller expects. Regardless, moving to RC1 for now

mboersma commented 8 years ago

This needs a rebase and should be merged for rc1 or probably just closed.ping @arschles @helgi

helgi commented 8 years ago

@mboersma There is no corresponding change for the Controller. I spent a bit of time before on this and the migration path wasn't one I could get to work. Do you want to try to do it? :P

mboersma commented 8 years ago

@helgi this just seemed like a potential non-backward-compatible change that I wanted to resolve before rc1. The original justification in deis/controller#278 doesn't sound urgent:

This field when blank is considered false, and true when non-blank. We should just change it to a boolean now that we're breaking APIs.

Unless @bacongobbler thinks the blank/non-blank boolean behavior is potentially buggy, and we can safely make the change for rc1, I think we should close this and the originating issue.

helgi commented 8 years ago

@mboersma it was more a "now is the time to break API if ever there was one"

arschles commented 8 years ago

Anyone have a consensus on what to do here? FWIW since this needs a rebase and we also need to implement the corresponding change in the controller, my opinion is this patch is more trouble than it's worth.

bacongobbler commented 8 years ago

Very low priority to fix. It's more of a nitpick since this behaviour of true/false in a string works, just not optimal. If it's too much of a pain to fix then we can come back to this in v3

arschles commented 8 years ago

ok, moving this out of a milestone completely. can revisit after v2 is released

Joshua-Anderson commented 8 years ago

I like this change, however, it would now need to happen in the controller SDK. @arschles Can this be closed?

I'm still in favor of making it, and I think we could do it in a backward compatible way.

  1. We migrate the field in the controller to a bool, and update the SDK to also use a bool.
  2. The controller could by updated to accept anything truthy for the field, so it would accept either the string or the bool.

Alternatively, we could say that the hooks api is an internal api and is therefore not constrained by semver, so we could make backwards incompatible changes to it.

arschles commented 8 years ago

@Joshua-Anderson yes, this can be closed and so can the issue IMO. I opened this PR for exemplary purposes only. Note that if we close this, we can (but don't have to) close #96 and its companion deis/controller#278