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.

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

Adding default to read fog credentials #1458

Merged
merged 1 commit into from
Nov 13, 2017
Merged

Adding default to read fog credentials #1458

merged 1 commit into from
Nov 13, 2017

Conversation

cdenneen
Copy link
Contributor

@tvpartytonight Added to presets to read from fog credentials in order to disable in beaker-aws

@puppetlabs-jenkins
Copy link
Contributor

Can one of the admins verify this patch?

tvpartytonight
tvpartytonight reviewed Sep 26, 2017
View reviewed changes
lib/beaker/options/presets.rb Outdated
@@ -38,6 +38,7 @@ class Presets
:test_tag_or => ['BEAKER_TEST_TAG_OR'],
:test_tag_exclude => ['BEAKER_EXCLUDE_TAG', 'BEAKER_TEST_TAG_EXCLUDE'],
:run_in_parallel => ['BEAKER_RUN_IN_PARALLEL'],
:use_fog_credentials => 'true',
Copy link
Contributor

Choose a reason for hiding this comment

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

@cdenneen this should be added to the list of options in the presets method down below in this file. Additionally, it should be just true, not 'true'.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

@cdenneen it looks like it's still a string here, is there a reason for that/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevpl ENV spec requires strings (doesn't work with bool).
The real preset is a bool:

https://github.com/cdenneen/beaker/blob/1b7ff68dab806dce9ebfce928e3e42f2dd8f1681/lib/beaker/options/presets.rb#L197

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would it be better if I just removed it from there or should we be adding a BEAKER_USE_FOG ?

@cdenneen
Copy link
Contributor Author

@tvpartytonight I originally tried boolean on line 41 but that doesn't work (fails all rspec tests) I believe it's explicitly looking for strings for everything.
I can add to line 197 below and think the boolean will work there.
Feel free to try changing 41 to a boolean to see the errors that will occur.

@tvpartytonight
Copy link
Contributor

@cdenneen it's explicitly looking for strings because it is loading it from the environment; the presets method is a hash, so you should have no trouble loading it in as a true class.

@tvpartytonight
Copy link
Contributor

@cdenneen last comment still current.

@kevpl
Copy link
Contributor

kevpl commented Oct 23, 2017

last comment still current.

@tvpartytonight tvpartytonight mentioned this pull request Oct 23, 2017
Closed
@cdenneen
Copy link
Contributor Author

@tvpartytonight @kevpl not following ... this is bool in latest commit.

@cdenneen cdenneen force-pushed the BKR-1199-read_fog_credentials branch from 2ef48df to 1b7ff68 Compare October 24, 2017 15:05
cdenneen
cdenneen commented Oct 24, 2017
View reviewed changes
Copy link
Contributor Author

@cdenneen cdenneen left a comment

Choose a reason for hiding this comment

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

Can one of the admins verify this patch?

lib/beaker/options/presets.rb Outdated
@@ -38,6 +38,7 @@ class Presets
:test_tag_or => ['BEAKER_TEST_TAG_OR'],
:test_tag_exclude => ['BEAKER_EXCLUDE_TAG', 'BEAKER_TEST_TAG_EXCLUDE'],
:run_in_parallel => ['BEAKER_RUN_IN_PARALLEL'],
:use_fog_credentials => 'true',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

kevpl
kevpl suggested changes Oct 30, 2017
View reviewed changes
lib/beaker/options/presets.rb Outdated
@@ -38,6 +38,7 @@ class Presets
:test_tag_or => ['BEAKER_TEST_TAG_OR'],
:test_tag_exclude => ['BEAKER_EXCLUDE_TAG', 'BEAKER_TEST_TAG_EXCLUDE'],
:run_in_parallel => ['BEAKER_RUN_IN_PARALLEL'],
:use_fog_credentials => 'true',
Copy link
Contributor

Choose a reason for hiding this comment

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

@cdenneen it looks like it's still a string here, is there a reason for that/

@cdenneen cdenneen force-pushed the BKR-1199-read_fog_credentials branch from 1b7ff68 to 645bc79 Compare October 30, 2017 19:00
cdenneen
cdenneen commented Oct 30, 2017
View reviewed changes
Copy link
Contributor Author

@cdenneen cdenneen left a comment

Choose a reason for hiding this comment

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

Removed the ENV test... so only one is the boolean at the bottom

lib/beaker/options/presets.rb Outdated
@@ -38,6 +38,7 @@ class Presets
:test_tag_or => ['BEAKER_TEST_TAG_OR'],
:test_tag_exclude => ['BEAKER_EXCLUDE_TAG', 'BEAKER_TEST_TAG_EXCLUDE'],
:run_in_parallel => ['BEAKER_RUN_IN_PARALLEL'],
:use_fog_credentials => 'true',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevpl ENV spec requires strings (doesn't work with bool).
The real preset is a bool:

https://github.com/cdenneen/beaker/blob/1b7ff68dab806dce9ebfce928e3e42f2dd8f1681/lib/beaker/options/presets.rb#L197

lib/beaker/options/presets.rb Outdated
@@ -38,6 +38,7 @@ class Presets
:test_tag_or => ['BEAKER_TEST_TAG_OR'],
:test_tag_exclude => ['BEAKER_EXCLUDE_TAG', 'BEAKER_TEST_TAG_EXCLUDE'],
:run_in_parallel => ['BEAKER_RUN_IN_PARALLEL'],
:use_fog_credentials => 'true',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

would it be better if I just removed it from there or should we be adding a BEAKER_USE_FOG ?

kevpl
kevpl approved these changes Nov 6, 2017
View reviewed changes
Copy link
Contributor

@kevpl kevpl left a comment

Choose a reason for hiding this comment

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

looks good to me, but I think @tvpartytonight is the most familiar with :use_fog_credentials on our team, so I'd like to see him take a look before merging this in.

@tvpartytonight tvpartytonight merged commit b1f8ceb into voxpupuli:master Nov 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tvpartytonight tvpartytonight tvpartytonight left review comments

@kevpl kevpl kevpl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cdenneen @puppetlabs-jenkins @tvpartytonight @kevpl