Skip to content
This repository was archived by the owner on Jan 8, 2019. It is now read-only.

Refactor dir creation #1267

Merged
merged 4 commits into from
Sep 10, 2018
Merged

Conversation

macmaster
Copy link
Contributor

We should consolidate all of the dir creation scripts into one.

@macmaster macmaster requested a review from vt0r August 30, 2018 14:44
@@ -3,7 +3,7 @@ module Helper
require 'yaml'
include Chef::Mixin::ShellOut

def dir_creation(mode, dirs, home, perms)
def dir_creation(mode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment to describe usage

Copy link
Contributor

Choose a reason for hiding this comment

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

Also change mode to something_else

node['bcpc']['hadoop']['hdfs']["#{dir}_ns_quota"] ||
node['bcpc']['hadoop']['hdfs'][mode]['ns_quota']
}
]
end.to_h
Copy link
Contributor

Choose a reason for hiding this comment

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

Move the hash merge out of the library and into recipe(s) to make rspec/chefspec work as expected when verifying the hash.

# Default HDFS User Directory Configuration
default['bcpc']['hadoop']['hdfs']['user'].tap do |user|
user['home'] = '/user'
user['group'] = 'supergroup'
Copy link
Contributor

Choose a reason for hiding this comment

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

supergroup --> hdfs (via some attribute if possible).

user['dirinfo'] = {
## Example dir_info entry:
# user_dirname: {
# owner: 'user',
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarify owner will always be set to the user we pass and remove this example from the comment.

## Example dir_info entry:
# group_dirname: {
# owner: 'user',
# group: 'group',
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarify group will always be set to the group we pass, and remove this example from the comment.

@@ -22,16 +16,12 @@
import org.apache.hadoop.fs.permission.FsPermission
import org.apache.hadoop.hdfs.protocol.HdfsConstants

usage = 'hbase shell '\
'create_dirs.rb [user|groups] dir_file quota_yml home_dir perms'
usage = "hbase shell #{$FILENAME} home_dir dirinfo_file"
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename this from home_dir to some generic directory like name, please.

'quota_file' => (ARGV[2] or raise "#{banner} quota_yml"),
'home' => (ARGV[3] or raise "#{banner} home_dir"),
'perms' => (ARGV[4] or raise "#{banner} perms")
'home' => (ARGV[0] or raise "#{banner} home_dir"),
Copy link
Contributor

Choose a reason for hiding this comment

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

directory or similar

puts "dir cache(#{dirs.length}): #{dirs}"
# dir creation configuration attributes
dirinfo = node['bcpc']['hadoop']['hdfs'][mode]['dirinfo']
home = node['bcpc']['hadoop']['hdfs'][mode]['home']
Copy link
Contributor

Choose a reason for hiding this comment

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

top_level_dir or something. Above we said directory, so sorry about that.

# Create directories for existing LDAP users and role accounts.
dirinfo = node.default['bcpc']['hadoop']['hdfs']['user']['dirinfo']
(cluster_users + cluster_roles).each do |user|
dirinfo[user] = deep_merge(dirinfo[user], { owner: user })
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned above, please clarify that owner will always be set to what is passed in the name of the resource.

# Create directories for existing LDAP groups.
dirinfo = node.default['bcpc']['hadoop']['hdfs']['groups']['dirinfo']
(cluster_groups).each do |group|
dirinfo[group] = deep_merge(dirinfo[group], { group: group })
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned above, please clarify that group will always be set to what is passed in the name of the resource.

@macmaster macmaster force-pushed the refactor-dir-creation branch 3 times, most recently from c68197c to 3efdde5 Compare August 31, 2018 01:19
@macmaster macmaster force-pushed the refactor-dir-creation branch from 3efdde5 to 840d44e Compare August 31, 2018 15:45
Copy link
Member

@vt0r vt0r left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@vt0r vt0r merged commit 8e5352f into bloomberg:master Sep 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants