Skip to content
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

Added unlock_keychain action #580

Closed
wants to merge 6 commits into from
Closed

Added unlock_keychain action #580

wants to merge 6 commits into from

Conversation

xfreebird
Copy link
Contributor

Added a new action unlock_keychain. It allows the user to unlock a given keychain, also it adds it to the keychain search list.

This is useful because it doesn't require to set a specific keychain as the default one, which might make issues when some program tries to access specific items from former keychains while being locked.

end
end

return false

Choose a reason for hiding this comment

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

Redundant return detected.

module Actions
class UnlockKeychainAction < Action
def self.run(params)
@keychain_path = params[:path]
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to use @keychain_path? You could pass the keychain path from the run method to the other methods, right?
I'd prefer to not store anything in the action itself.

@KrauseFx
Copy link
Member

KrauseFx commented Sep 7, 2015

This looks like a great addition, thank you for your contribution @xfreebird 👍 I added some notes.

Also, could you add 1 or 2 simple test cases? I saw you have the commands as the last line of the run method, which makes it very easy to test.

Take a look at the other action specs, let me know if you need help with that. Run the tests using rspec

Thanks again for your great contribution 🚀

expect(result[3]).to include '-u'
expect(result[3]).to include '~/Library/Keychains/test.keychain'
expect(result[0]).to eq "security unlock-keychain -p testpassword #{expanded_path}"
expect(result[1]).to eq "security set-keychain-settings #{expanded_path}"

Choose a reason for hiding this comment

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

Line is too long. [81/80]

@KrauseFx
Copy link
Member

KrauseFx commented Sep 7, 2015

Oh, sorry about those @houndci messages, somehow it didn't fetch the latest style configuration.

@KrauseFx
Copy link
Member

KrauseFx commented Sep 7, 2015

Do you need help with adding the tests?

end
end
end
end

Choose a reason for hiding this comment

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

Final newline missing.

end
end

return ""

Choose a reason for hiding this comment

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

Redundant return detected.

@xfreebird
Copy link
Contributor Author

sorry about that, I added the wrong content to the spec file :)

Anyway I still don't quite good understand what exactly does runner.execute returns in spec ?
Because I was expecting to see there all the commands that were executed, but somehow it doesn't contain.

I'm very new to ruby, so please understand my simple mistakes.

describe "Unlock keychain Integration" do

it "works with path and password and existing keychain" do
keychain_path = Tempfile.new('foo').path

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@KrauseFx
Copy link
Member

KrauseFx commented Sep 8, 2015

Thanks for your contribution @xfreebird, this is now merged and will be in the next release later today 🚀

@KrauseFx KrauseFx closed this Sep 8, 2015
@xfreebird
Copy link
Contributor Author

Thanks @KrauseFx !

@fastlane fastlane locked and limited conversation to collaborators Feb 4, 2017
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.

None yet

3 participants