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

Updated so that the credentials file is only interogated if it exists #42

Merged
merged 2 commits into from
Oct 9, 2017

Conversation

russellseymour
Copy link
Contributor

Fixes #39

Signed-off-by: Russell Seymour russell.seymour@turtlesystems.co.uk

Fixes #39

Signed-off-by: Russell Seymour <russell.seymour@turtlesystems.co.uk>
@russellseymour
Copy link
Contributor Author

Fixes issue when only using environment variables
It was mistakenly checking for the credentials file

/cc @adamleff @chris-rock

Copy link

@adamleff adamleff left a comment

Choose a reason for hiding this comment

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

Seems like a reasonable change to me!

My only suggestion: when a variable (like @credentials) contains contents, I tend to like to set the variable to nil instead of false if the file doesn't exist or can't be read. Then down around like 60, it would say unless @credentials.nil? rather than if @credentials. Perhaps it's personal preference, but I think it's clearer.

Alternatively, and mostly because I like to break things up into small methods to make testing easier, I would consider creating a method credentials_file_exists? and using that in lines 29 and line 60 instead.

But this is totally fine as-is as far as I'm concerned. :)

Copy link
Contributor

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

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

I would like to see the improvement mentioned by @adamleff implemented, since we assign false to a variable that holds the content of the ini file. Therefore we really should just check for nil.

@gianlucaciocci
Copy link

the pull request #40 and this one are a duplicate, both are trying to fix the same issue. Could you please advice the way to go? If we decide to go with this implementation I'll delete the fix included into the pull request mentioned. Please let me know, thanks.

Signed-off-by: Russell Seymour <russell.seymour@turtlesystems.co.uk>
@russellseymour
Copy link
Contributor Author

I have modified the @credentials so that it is nil if there is no credentials file.

I have not added tge method credentials_file_exists? because the file is only checked for existence in the initialize method. The next check is to detetermine if the subscription is set in the contents of the credentials file which has be loaded and not the file path.

Sorry for the delay in sorting this, I need to get my GitHub alerts sorted.

/cc @adamleff @chris-rock @gianlucaciocci

Copy link
Contributor

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

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

Thank you @russellseymour

@chris-rock chris-rock merged commit e32225c into master Oct 9, 2017
@chris-rock chris-rock deleted the russellseymour/env-vars branch October 9, 2017 10:54
@Tushard8
Copy link

Guy's, I am facing related issue after recent changes in repo. I am a tester and using this repo to write tests for checking Azure VM's hosted in our Infra.

https://github.com/chef/inspec-azure/issues/58

What am I doing wrong here ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants