Add platform uuid information. #270
Conversation
jquick
force-pushed
the
jq/platform_uuid
branch
3 times, most recently
from
08aa444 to
1fcfdb0
Compare
|
I think I’m going to move the logic to the detect helpers. |
lib/train/platforms/uuid.rb
Outdated
| return uuid_from_string(result.stdout.chomp) if result.exit_status == 0 && !result.stdout.empty? | ||
| end | ||
|
|
||
| raise 'Could not fine platform uuid! please set a uuid_command for your platform' |
There was a problem hiding this comment.
s/fine/find/ =^)
Addendum: Also, in the second sentence, capitalize "please", please, and punctuate.
lib/train/platforms/uuid.rb
Outdated
| end | ||
|
|
||
| def azure | ||
| # use subscription_id or tenant_id |
There was a problem hiding this comment.
Redundant comment, yeah? Also the kind that becomes a lie when the implementation changes.
lib/train/platforms/uuid.rb
Outdated
| windows | ||
| elsif @platform.name == 'aws' | ||
| aws | ||
| elsif @platform.name == 'azure' |
There was a problem hiding this comment.
Why aren't there predicate methods for aws and azure already?
There was a problem hiding this comment.
due to ? only getting created for platform families. I have been thinking about this and I will wrap aws/azure in their own families so we get them.
There was a problem hiding this comment.
Gotcha. Yeah it'd be great to have the uniform api. Then the case statement could be collapsed. =^)
lib/train/platforms/uuid.rb
Outdated
| raise "Cannot write uuid to `#{local_uuid_path}\\machine-uuid`" if result.exit_status !=0 | ||
| end | ||
| uuid | ||
| end |
There was a problem hiding this comment.
I see about 5 methods lurking inside this one. Is that sort of abstraction unconventional in this project?
There was a problem hiding this comment.
yeah at this point they should be divided up as sub private methods. Thanks for this, ill grab it when I move them over.
jquick
force-pushed
the
jq/platform_uuid
branch
3 times, most recently
from
ac9ac65 to
b4c06f7
Compare
There was a problem hiding this comment.
Looks great @jquick! I like how you moved the logic around.
I only have a few nitpicks then this will be good to go. Also, I didn't mention it because it's not a big deal, but if you could rewrite your it blocks to read as a sentence that would make me 😄 e.g. it 'returns an account id'
lib/train/platforms/uuid.rb
Outdated
| return uuid_from_string(result.stdout.chomp) if result.exit_status == 0 && !result.stdout.empty? | ||
| end | ||
|
|
||
| raise 'Could not fine platform uuid! please set a uuid_command for your platform' |
There was a problem hiding this comment.
Could we capitalize please
| # fallback to user location | ||
| warn 'Cannot write uuid to `/etc/machine-uuid`, falling back to ~/.local/etc/machine-uuid instead.' | ||
| result = @backend.run_command("mkdir -p #{ENV['HOME']}/.local/etc; echo \"#{uuid}\" > #{ENV['HOME']}/.local/etc/machine-uuid") | ||
| raise 'Cannot write uuid to `~/.local/etc/machine-uuid`.' unless result.exit_status.zero? |
There was a problem hiding this comment.
Since we surround /etc/machine-uuid with backticks, can we do the same for ~/.local/etc/machine-uuid as well? Also, can we interpolate this in some way so that the home directory is expanded and shown to the user?
| uuid_from_string(result.stdout.chomp) if result.exit_status.zero? && !result.stdout.empty? | ||
| end | ||
|
|
||
| def uuid_from_string(string) |
There was a problem hiding this comment.
Could we add a few comments explaining the conversion here?
| unless result.exit_status.zero? | ||
| # fallback to user location | ||
| local_uuid_path = "#{ENV['HOMEDRIVE']}#{ENV['HOMEPATH']}\\.system\\machine-uuid" | ||
| warn "Cannot write uuid to `#{ENV['SYSTEMROOT']}\\machine-uuid`, falling back to #{local_uuid_path} instead." |
There was a problem hiding this comment.
Can we surround #{local_uuid_path} with backticks?
Signed-off-by: Jared Quick <[email protected]>
jquick
force-pushed
the
jq/platform_uuid
branch
3 times, most recently
from
63f5c0c to
40ac05d
Compare
Signed-off-by: Jared Quick <[email protected]>
jquick
force-pushed
the
jq/platform_uuid
branch
from
40ac05d to
3828ed7
Compare
There was a problem hiding this comment.
You did the thing with the it blocks! Day = Made.
Thanks @jquick
|
Do not merge yet, need to add the new https://github.com/chef/chef/pull/7016/files chef_guid |
Signed-off-by: Jared Quick <[email protected]>
|
Added, should be good to go! |
jquick
force-pushed
the
jq/platform_uuid
branch
2 times, most recently
from
9f2c5ff to
1c2df9c
Compare
Signed-off-by: Jared Quick <[email protected]>
jquick
force-pushed
the
jq/platform_uuid
branch
from
1c2df9c to
1479962
Compare
There was a problem hiding this comment.
Thank you @jquick for this great feature. I like that we focus on read-only mechanisms.
This change adds a uuid option for platforms. This is either pulled from a unique identifier already on the box or by creating one. You also have the option of setting a command to run for a unique id during the platform detect block.
Currently how this is written is we will only run the UUID commands on demand when called upstream. This is to reduce the number of calls when detecting a OS.
Fixes #264
Signed-off-by: Jared Quick [email protected]