Skip to content
This repository has been archived by the owner on Jun 27, 2019. It is now read-only.

read deploy key from env if available #239

Merged
merged 3 commits into from
Feb 15, 2018
Merged

read deploy key from env if available #239

merged 3 commits into from
Feb 15, 2018

Conversation

xavdid
Copy link
Contributor

@xavdid xavdid commented Feb 14, 2018

addresses PDE-59.

Fixed a potential bug on the way: the only place readCredentials actually returned a value, it was expect to return an object with the deployKey key. The function also takes a param if you want to pass in a key, and would echo it. The issue was that it would echo the value, not wrapped in the object as was expected.

To that end, nowhere in the codebase actually passes in a credentials value. That function signature complicates some calls (ie: readCredentials(null, false)) and unless it's there for a reason (@bryanhelmig; sorry for the double ping) I propose we remove the parameter entirely.

@xavdid xavdid requested a review from eliangcs February 14, 2018 06:51
Copy link
Member

@eliangcs eliangcs left a comment

Choose a reason for hiding this comment

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

Pulled and tested. Works perfectly! I also agree that removing the echo part of readCredentials would make the code clearer.

@xavdid xavdid merged commit 6abe631 into master Feb 15, 2018
@xavdid xavdid deleted the deploy-key-env branch February 15, 2018 08:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants