Skip to content
This repository has been archived by the owner on May 6, 2020. It is now read-only.

feat(client): use unix pipes with config:push #72

Merged
merged 1 commit into from
Feb 9, 2016

Conversation

Joshua-Anderson
Copy link
Contributor

This was briefly discussed as a V2 thing in deis/deis#4675 and deis/deis#4697.

I personally like it cause it's more unixy, but I understand if this is rejected because this is really a personal preference thing 😃.

If there is interest I will make the same change for config:pull.


Usage: deis config:push [options]

Options:
-a --app=<app>
the uniquely identifiable name for the application.
-p <path>, --path=<path>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we support both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There isn't any technical reason we can't. I figured it would be confusing to offer two ways to do the same thing, but I don't have any strong feelings either way.

Copy link
Member

Choose a reason for hiding this comment

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

IMO we should support both, but doesn't have to be blocking for this PR

@technosophos
Copy link
Member

@Joshua-Anderson We're getting close to freezing an Alpha of Deis v2. In your opinion, is this a risky one for us to try to merge at the last minute? Or should we wait until after Alpha.

@Joshua-Anderson
Copy link
Contributor Author

@technosophos Works on my machine (Ubuntu 15.10) ™️ 😃

I think merging this more of a decision if this change is desirable (some people may prefer flags over pipes). It's a small scoped and pretty easily testable change, so it wouldn't be too much work to land ( I have no idea how soon you want to release or how busy you are 😃)

If this doesn't make it into the alpha, can backwards compatible changes still land in V2 before the GA release?

This change also needs a complimentary change to config:pull to support pipes as well, but I was hanging off on that until this change was approved or rejected. So releasing one without the other would leave an odd UX where pull and push behaved differently.

@arschles
Copy link
Member

@Joshua-Anderson I think it's wise to implement config:pull in this patch as well

@mboersma mboersma added this to the v2.0-beta1 milestone Jan 14, 2016
@Joshua-Anderson
Copy link
Contributor Author

I think it's wise to implement config:pull in this patch as well

Ok. Sounds good! I'm super busy right now, but if I have some time I'll fix it up.

@helgi helgi removed this from the v2.0-beta1 milestone Feb 5, 2016
@Joshua-Anderson
Copy link
Contributor Author

@arschles I updated this PR to support both pipes and command line arguments.

@arschles
Copy link
Member

arschles commented Feb 8, 2016

@Joshua-Anderson cool. My LGTM1 stands, but it looks like you need a rebase now

@Joshua-Anderson
Copy link
Contributor Author

😦 As a side effect of enabling required status (which I am totally 👍 on), every time there is a commit on master, PRs are required to update before merging (to prevent accidental test breakage), which is a little annoying.

Joshua-Anderson added a commit that referenced this pull request Feb 9, 2016
feat(client): use unix pipes with config:push
@Joshua-Anderson Joshua-Anderson merged commit 4b8b388 into deis:master Feb 9, 2016
@Joshua-Anderson Joshua-Anderson deleted the use-pipes branch February 9, 2016 06:59
@bacongobbler
Copy link
Member

hmm, that might be why everything needs to be rebased all the time. deis/deis does not have that enabled so that might be the bug. Definitely annoying :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants