这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@u2mejc
Copy link
Contributor

@u2mejc u2mejc commented Nov 7, 2015

- fixes issue dokku#1658
- add better output for argument files missing
- don't delete (move) the certs passed in
Copy link
Member

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?

Copy link
Contributor Author

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!!! 😉

@michaelshobbs
Copy link
Member

I'm not certain this fixes #1658. This plugin could definitely use some tests anyway, though. Care to take a stab at that and include the case from #1658? You can use the test certs in tests/server_ssl*.tar.

@michaelshobbs
Copy link
Member

The way I read it, we are working in a temp folder and not on the input. Maybe I misread it?

@u2mejc
Copy link
Contributor Author

u2mejc commented Nov 7, 2015

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:
dokku certs:add ruby-sample /home/myuser/server.crt /home/myuser/server.key
CRT_FILE=$3

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.

@michaelshobbs
Copy link
Member

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?

@u2mejc
Copy link
Contributor Author

u2mejc commented Nov 7, 2015

It's 5:59 on friday, happy to! 👍

@michaelshobbs
Copy link
Member

I definitely did not mean right this moment sir. 😄️

@hhff
Copy link
Contributor

hhff commented Nov 7, 2015

thanks duuuuuudes

@josegonzalez
Copy link
Member

@u2mejc bump I also don't see how it solves the bug.

@u2mejc
Copy link
Contributor Author

u2mejc commented Nov 10, 2015

Bummer, @michaelshobbs @josegonzalez when you cloned and tested it, what was the results of your tests?

@josegonzalez
Copy link
Member

I didn't try it, I'm just saying it doesn't look like it does.

Copy link
Member

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

josegonzalez added a commit that referenced this pull request Nov 11, 2015
Fix dokku certs:add file input bug
@josegonzalez josegonzalez merged commit ffbb2f3 into dokku:master Nov 11, 2015
@michaelshobbs
Copy link
Member

@josegonzalez no tests!! why u merge bro!? 😄

@u2mejc
Copy link
Contributor Author

u2mejc commented Nov 11, 2015

Sorry I didn't have a chance to respond sooner! I'll work on tests and submit a separate PR.

@u2mejc u2mejc deleted the u2mejc-certs branch November 11, 2015 01:55
@josegonzalez
Copy link
Member

@michaelshobbs I'm too trigger happy, sorry :(

@josegonzalez
Copy link
Member

I saw you say you understood it so I was like LG2ME

@michaelshobbs
Copy link
Member

I was just so excited I finally saw the feex 😜️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants