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

Fix aws sdk dependencies #335

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rajiv-g
Copy link
Contributor

@rajiv-g rajiv-g commented Apr 8, 2019

Pull Request Checklist

Is this in reference to an existing issue?
Solve #293

General

  • Update Changelog following the conventions laid out here

  • Update README with any necessary configuration snippets

  • Binstubs are created if needed

  • RuboCop passes

  • Existing tests pass

New Plugins

  • Tests

  • Add the plugin to the README

  • Does it have a complete header as outlined here

Purpose

Known Compatibility Issues

@rajiv-g
Copy link
Contributor Author

rajiv-g commented Apr 8, 2019

@majormoses Any idea for test cases?

@majormoses
Copy link
Member

@rajiv-g thanks for putting this together, honestly its gonna be a beast to test and I would not expect a quick merge. I would say the best place to start would be to spin up a fresh sensu-client install the new version and then run every check you are currently using and post it back here. Including a testing artifact will certainly give us more confidence about it not breaking anything. Perhaps as an intermediary step we could do is update the requirements in each of the files themselves while leaving the gemspec alone so we can indicate which ones have been tested with the pared down dependency list. This allows us to test the waters a bit easier with less risk of breaking existing setups. WDYT?

@rajiv-g
Copy link
Contributor Author

rajiv-g commented Apr 11, 2019

@majormoses Thanks for your suggestion. So we are only going to add dependencies to the files that are tested. I have already added dependencies to each files tentatively. We can remove that one & add dependencies after testing each file?
Will include testing articfact as much as possible when I have some time.

@majormoses
Copy link
Member

Ya if you want to leave them all and report back after testing each one but my thought was if your not using ever check, metric script, and handler testing all the resources might be a bit difficult. With that in mind I was thinking that we leave the gem still in the gemspec and make a minor release (no breaking changes) with the ones we are able to test. If several people do this with all the checks they use we can likely determine if its a safe upgrade and can get there without risking breaking anyone. We do have some coverage with unit tests but I tend to trust unit tests less than integration tests.

@majormoses
Copy link
Member

I suspect we need to include aws-sdk-core somewhere but I have not tested yet.

@majormoses
Copy link
Member

I guess we dont need but I think we should lock aws-sdk-core.

@majormoses majormoses mentioned this pull request May 9, 2019
8 tasks
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.

None yet

2 participants