-
Notifications
You must be signed in to change notification settings - Fork 143
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
base: master
Are you sure you want to change the base?
Fix aws sdk dependencies #335
Conversation
@majormoses Any idea for test cases? |
@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? |
@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? |
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. |
I suspect we need to include |
I guess we dont need but I think we should lock |
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