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

Fixed silo profile creation #185

Merged
merged 3 commits into from
Sep 29, 2015
Merged

Fixed silo profile creation #185

merged 3 commits into from
Sep 29, 2015

Conversation

gschneider-r7
Copy link
Contributor

Updated the error message to check for update vs create condition. Resolves #160

Updated the error message to check for update vs create condition.
@gschneider-r7 gschneider-r7 added this to the 3.0 milestone Aug 26, 2015
@@ -107,7 +107,7 @@ def save(connection)
begin
update(connection)
rescue APIError => error
raise error unless (error.message =~ /A silo profile .* does not exist./)
raise error unless (error.message =~ /The silo profile .* does not exist/)
Copy link
Contributor

Choose a reason for hiding this comment

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

So #160's root cause is that the message for the error changed ever so slightly, and your fix is to ... change it here ever so slightly. Why not:

raise error unless (error.message =~ /\S+ silo profile .* does not exist/

or something similarly more flexible?

(Also, OT, but Ideally there'd be a dedicated exception class for this so you could check the type rather than the message, but this is not an uncommon pattern, especially since the Nexpose gem is somewhat at the mercy of what Nexpose itself responds with)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some day we'll have a fully RESTful JSON API to replace this that returns appropriate status codes instead of messages that must be parsed. 😃

I am not too concerned about flexibility since this message really shouldn't change again (or if it does the above should come true first).

Copy link
Contributor

Choose a reason for hiding this comment

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

I would agree with @jhart-r7 - might as well just not worry about it again until we have to change it to handle status codes.

@@ -107,7 +107,7 @@ def save(connection)
begin
update(connection)
rescue APIError => error
raise error unless (error.message =~ /A silo profile .* does not exist./)
raise error unless (error.message =~ /\S+ silo profile .* does not exist/)
Copy link
Contributor

Choose a reason for hiding this comment

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

this might work better.

/silo profile(\S*|.*?)does not exist/i

Should match all of these, according to http://rubular.com/

aSa silo profile does not exist
silo profile does not exist
A silo profile #1223-a does not exist
A silo profile 123 does not exist
silo profile abcd does not exist
A Silo Profile Does Not Exist

@gschneider-r7 gschneider-r7 modified the milestone: 3.0 Sep 21, 2015
gschneider-r7 added a commit that referenced this pull request Sep 29, 2015
@gschneider-r7 gschneider-r7 merged commit 0856281 into master Sep 29, 2015
@gschneider-r7 gschneider-r7 deleted the fix_silo_profile branch September 29, 2015 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants