Skip to content

Added list-keys command, with associated spec #177

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 28, 2014
Merged

Added list-keys command, with associated spec #177

merged 1 commit into from
Oct 28, 2014

Conversation

david-martin
Copy link
Contributor 

./bin/gitlab-keys list-keys

key-123 AAAAB3NzaDAxx2E
key-456 AAAAB3NzaDAxx2F

houndci-bot
houndci-bot reviewed Sep 16, 2014
View reviewed changes
lib/gitlab_keys.rb
keys = ""
File.readlines(auth_file).each do |line|
# key_id & public_key
# command="/home/git/gitlab-shell/bin/gitlab-shell key-741",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty ssh-rsa AAAAB3NzaDAxx2E\n
Copy link

Choose a reason for hiding this comment

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

Line is too long. [155/80]

@jvanbaarsen
Copy link
Contributor

@david-martin Can you please fix the comments placed hy @houndci ? You can ignore the Double quote ones.

houndci-bot
houndci-bot reviewed Sep 17, 2014
View reviewed changes
spec/gitlab_keys_spec.rb
@@ -41,6 +41,19 @@
end
end

describe :list_keys do
let(:gitlab_keys) {
Copy link

Choose a reason for hiding this comment

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

Avoid using {...} for multi-line blocks.

@Razer6
Copy link
Member

Razer6 commented Sep 17, 2014

Can you also please change all double quote strings to single quoted ones?

@david-martin
Copy link
Contributor Author

@Razer6 conflicting pov from you ,@jvanbaarsen & the hound.
Whats the convention here?

@Razer6
Copy link
Member

Razer6 commented Sep 17, 2014

@david-martin Always use single quotes. Hound is currently misconfigured. See #178 for the pending PR

@david-martin
Copy link
Contributor Author

@Razer6 double quotes are required for string interpolation in 2 places. I'm not going to change these.
There are numerous example in the current code base where the houndci styleguide is not being followed. Perhaps a mass update should be done, or modify your styleguide to suit the existing code rather than mixing and merging styles from this point forward

@Razer6
Copy link
Member

Razer6 commented Sep 17, 2014

double quotes are required for string interpolation in 2 places. I'm not going to change these.

That's fine!

We know, that not 100% of code matches hound style guide. But we enabled hound, that at least new code follows the style guide.

@david-martin
Copy link
Contributor Author

@Razer6 @jvanbaarsen Where are we with getting this PR merged back?
Are there any remaining actions on my part?

Razer6
Razer6 reviewed Oct 15, 2014
View reviewed changes
lib/gitlab_keys.rb
@@ -38,6 +39,19 @@ def add_key
true
end

def list_keys
$logger.info "Listing all keys"
Copy link
Member

Choose a reason for hiding this comment

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

Can you change strings to single line quotes?

houndci-bot
houndci-bot reviewed Oct 15, 2014
View reviewed changes
lib/gitlab_keys.rb
@@ -38,6 +39,19 @@ def add_key
true
end

def list_keys
$logger.info 'Listing all keys'
keys = ""
Copy link

Choose a reason for hiding this comment

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

Prefer single-quoted strings when you don't need string interpolation or special symbols.

@david-martin
Added list-keys command and spec
89d8af4 
Removed puts and tidied up regex

Address the hound

Address the hound, again

Use single quotes

Add back travis.yml file

Remove travis.yml, only keep on fh-master

Use single quotes

Use single quotes
@david-martin
Copy link
Contributor Author

@Razer6 done. houndci has quietened down now. Anything other changes required to get this merged?

@Razer6 Razer6 added this to the 7.5 milestone Oct 15, 2014
@Razer6
Copy link
Member

Razer6 commented Oct 15, 2014

@randx Can you take a look?

dzaporozhets added a commit that referenced this pull request Oct 28, 2014
@dzaporozhets
Merge pull request #177 from feedhenry/keys_list
3a16acf 
Added list-keys command, with associated spec
@dzaporozhets dzaporozhets merged commit 3a16acf into gitlabhq:master Oct 28, 2014
@grdryn grdryn deleted the keys_list branch September 21, 2017 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Ready to merge
Milestone
7.5
Development

Successfully merging this pull request may close these issues.
5 participants
@david-martin @jvanbaarsen @Razer6 @houndci-bot @dzaporozhets