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

Handled exception from vault? method #350

Merged
merged 5 commits into from
Aug 12, 2020
Merged

Handled exception from vault? method #350

merged 5 commits into from
Aug 12, 2020

Conversation

sanga1794
Copy link
Contributor

@sanga1794 sanga1794 commented Jun 10, 2020 •
edited
Loading

Signed-off-by: sanga17 [email protected]

Description

Fixed 'vault?' method should throw meaningful exception when no data bag exists

Related Issue

Fixed: #278

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commits have been signed-off for the Developer Certificate of Origin.

@sanga1794 sanga1794 requested a review from a team June 10, 2020 11:41
@sanga1794 sanga1794 changed the title Handled exception from vault? method WIP: Handled exception from vault? method Jun 10, 2020
@sanga1794 sanga1794 changed the title WIP: Handled exception from vault? method Handled exception from vault? method Jun 11, 2020
Vasu1105
Vasu1105 reviewed Jun 17, 2020
View reviewed changes
lib/chef-vault/item.rb Outdated
else
raise http_error
end
rescue Chef::Exceptions::ValidationFailed
Copy link

@Vasu1105 Vasu1105 Jun 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@sanga1794 we are raising Item not found exception in case of ValidationFailed ? So wouldn't it will raise the Item not found exception even though its a validation failure ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just took the reference from method self.load

chef-vault/lib/chef-vault/item.rb

Line 289 in 6428e99

def self.load(vault, name, opts = {})

tas50
tas50 reviewed Jul 29, 2020
View reviewed changes
lib/chef-vault/item.rb Outdated
else
raise http_error
end
rescue Chef::Exceptions::ValidationFailed
Copy link
Contributor

Choose a reason for hiding this comment

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

Does validation failing always indicate that the data bag isn't there? Would't that be the 404 you're rescuing above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes @tas50, in my case only rescuing 404 will be fine, will remove the remaining, thanks

@sanga1794 sanga1794 requested a review from a team as a code owner August 5, 2020 15:15
@sanga1794 sanga1794 force-pushed the sangmeshA/Fix_vault_method_should_throw_meaningful_exception_when_no_data_bag_exists branch from 9c0374d to 690cdd8 Compare August 5, 2020 15:17
kagarmoe
kagarmoe reviewed Aug 6, 2020
View reviewed changes
lib/chef-vault/item.rb Outdated
rescue Net::HTTPServerException => http_error
if http_error.response.code == "404"
raise ChefVault::Exceptions::ItemNotFound,
"#{vault}/#{name} could not be found"
Copy link

Choose a reason for hiding this comment

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

Consider "#{vault}/#{name} not found", unless we habitually use the polite subjunctive could not be in our output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @kagarmoe, will update it.

kagarmoe
kagarmoe reviewed Aug 6, 2020
View reviewed changes
Copy link

@kagarmoe kagarmoe left a comment

Choose a reason for hiding this comment

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

Left a style note, but I don't know the code base well enough to insist on it.

tas50
tas50 suggested changes Aug 10, 2020
View reviewed changes
Copy link
Contributor

@tas50 tas50 left a comment

Choose a reason for hiding this comment

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

pry stack explorer needs to be pinned so these tests will pass. Here's how we're doing it in ohai:

https://github.com/chef/ohai/blob/master/Gemfile#L25

@sanga1794
Copy link
Contributor Author

sanga1794 commented Aug 11, 2020 •
edited
Loading

pry stack explorer needs to be pinned so these tests will pass. Here's how we're doing it in ohai:

https://github.com/chef/ohai/blob/master/Gemfile#L25

Thanks @tas50, I'll update it

@tas50
Copy link
Contributor

tas50 commented Aug 11, 2020

@sanga1794 There's a simple chefstyle violation that should get corrected to get the tests green as well.

@sanga1794 sanga1794 force-pushed the sangmeshA/Fix_vault_method_should_throw_meaningful_exception_when_no_data_bag_exists branch from 4eeb109 to 452611e Compare August 11, 2020 08:45
@tas50 tas50 merged commit f95031d into chef:master Aug 12, 2020
@tas50
Copy link
Contributor

tas50 commented Aug 12, 2020

Thanks @sanga1794

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vasu1105 Vasu1105 Vasu1105 left review comments

@kagarmoe kagarmoe kagarmoe left review comments

@tas50 tas50 tas50 requested changes

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

Successfully merging this pull request may close these issues.

vault? method should throw meaningful exception when no data bag exists
4 participants
@sanga1794 @tas50 @Vasu1105 @kagarmoe