Conversation
micahlee
force-pushed
the
reset-password-rake-task
branch
from
ea8ef3b to
040c878
Compare
lib/tasks/reset_password.rake
Outdated
| require 'io/console' | ||
|
|
||
| namespace :role do | ||
| desc "Retrieve the API key for the given role" |
There was a problem hiding this comment.
The description and the task name address two different points (retrieve an API key, reset password). What do you think about combining these two outcomes in the description and task name?
There was a problem hiding this comment.
Ah, that's actually a copy/paste error from https://github.com/cyberark/conjur/blob/master/lib/tasks/role.rake I overlooked .
I will update this to:
desc "Reset the password and API key for the given role"
WDYT?
| desc "Retrieve the API key for the given role" | ||
| task :"reset-password", [:role_id] => [:environment] do |t, args| | ||
|
|
||
| role = Role.first(role_id: args[:role_id]) |
There was a problem hiding this comment.
Instead of Role, do we want to limit this to `User? If we are keeping this generic, should we skip password setting for hosts?
There was a problem hiding this comment.
Yes, that's a good call. I'll update this. Thanks!
![qlty-cloud-legacy[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
|
|
||
| namespace :role do | ||
| desc "Retrieve the API key for the given role" | ||
| task :"reset-password", [:role_id] => [:environment] do |t, args| |
There was a problem hiding this comment.
Complex method namespace(role)::task#reset-password (61.8)
| namespace :role do | ||
| desc "Retrieve the API key for the given role" | ||
| task :"reset-password", [:role_id] => [:environment] do |t, args| | ||
|
|
There was a problem hiding this comment.
Extra empty line detected at block body beginning.
|
|
||
| namespace :role do | ||
| desc "Retrieve the API key for the given role" | ||
| task :"reset-password", [:role_id] => [:environment] do |t, args| |
There was a problem hiding this comment.
Unused block argument - t. If it's necessary, use _ or _t as an argument name to indicate that it won't be used.
| role.credentials.rotate_api_key | ||
| role.credentials.save | ||
| end | ||
| rescue => err |
| exit(1) | ||
| end | ||
|
|
||
| password = IO::console.getpass("Enter new password: ") |
There was a problem hiding this comment.
Do not use :: for method calls.
|
Code Climate has analyzed commit 040c878 and detected 6 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 14.2% (50% is the threshold). This pull request will bring the total coverage in the repository to 89.8% (-1.6% change). View more on Code Climate. |
micahlee
force-pushed
the
reset-password-rake-task
branch
from
040c878 to
744af77
Compare
micahlee
force-pushed
the
reset-password-rake-task
branch
from
744af77 to
8e38d61
Compare
|
It seems like this could be really useful. Any reason it isn't merged? |
conjur-jenkins
force-pushed
the
master
branch
2 times, most recently
from
6566647 to
d65d499
Compare
conjur-jenkins
force-pushed
the
master
branch
from
d65d499 to
dbea020
Compare
conjur-jenkins
force-pushed
the
master
branch
from
dbea020 to
8031a2a
Compare
3 participants
Desired Outcome
Please describe the desired outcome for this PR. Said another way, what was
the original request that resulted in these code changes? Feel free to copy
this information from the connected issue.
Implemented Changes
Describe how the desired outcome above has been achieved with this PR. In
particular, consider:
Connected Issue/Story
Resolves #[relevant GitHub issue(s), e.g. 76]
CyberArk internal issue ID: [insert issue ID]
Definition of Done
At least 1 todo must be completed in the sections below for the PR to be
merged.
Changelog
CHANGELOG update
Test coverage
changes, or
Documentation
READMEs) were updated in this PRBehavior
Security