-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Fix dokku certs:add file input bug #1667
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
Conversation
u2mejc
commented
Nov 7, 2015
- fixes issue dokku certs:add only supports tarball #1658
- add better output for argument files missing
- don't delete (move) the certs passed in
- fixes issue dokku#1658 - add better output for argument files missing - don't delete (move) the certs passed in
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 suppose it doesn't matter as $TEMP_DIR get removed but why change to cp?
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.
@michaelshobbs no you tell me, why you delete my certs bro? That was my only copy!!! 😉
|
The way I read it, we are working in a temp folder and not on the input. Maybe I misread it? |
|
ah sorry I understand your confusion now, github isn't showing you the whole file. The temp dir is for the tar file and unrelated to passing in file arguments from the command line. Here's an example: If you mv $CRT_FILE, your literally deleting the user's /home/myuser/server.crt. The verbiage add doesn't imply delete to the user and I can't think of a single program I would expect to delete my key and force me to retrieve a new copy from the CA. Does heroku delete the keys from your laptop when it copies to the server? I can see where that might be confusing. |
|
No I think the Heroku client just copies them up to the paas. I figured I just misread it. Still would be great to get at least one test written for each input type. i.e. file and tarball. Care to take a stab at that? |
|
It's 5:59 on friday, happy to! 👍 |
|
I definitely did not mean right this moment sir. 😄️ |
|
thanks duuuuuudes |
|
@u2mejc bump I also don't see how it solves the bug. |
|
Bummer, @michaelshobbs @josegonzalez when you cloned and tested it, what was the results of your tests? |
|
I didn't try it, I'm just saying it doesn't look like it does. |
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 see the fix now!!!
Fix dokku certs:add file input bug
|
@josegonzalez no tests!! why u merge bro!? 😄 |
|
Sorry I didn't have a chance to respond sooner! I'll work on tests and submit a separate PR. |
|
@michaelshobbs I'm too trigger happy, sorry :( |
|
I saw you say you understood it so I was like LG2ME |
|
I was just so excited I finally saw the feex 😜️ |