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 conservative dependencies to have proper "x.y" values #30

Merged
merged 4 commits into from
Dec 10, 2019

Conversation

zenspider
Copy link
Contributor

More followup to previous dependency problems.

Copy link
Collaborator

@clintoncwolfe clintoncwolfe left a comment

Choose a reason for hiding this comment

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

Because this is a high-profile and delicate dependency control file, could you take a moment and explain in greater detail for a broader audience the nature of the changes being introduced in the version pinning, and what motivates the changes?

Thank you!

@zenspider
Copy link
Contributor Author

Absolutely... The diff centers around my having done this the first time around incorrectly. All of the dependencies (almost all comments at this point) were written with ~> 1 version specifiers, which read "greater than or equal to one" when the value is 1. I should have written 1.0, which would have meant "Anything 1.x but less than 2." As such, the previous version was entirely unbounded and that's always going to be a problem when new versions come down the pipe (assuming aws follows semver, which I can only assume).

@zenspider zenspider changed the title Zenspider/more dep stuff Fix conservative dependencies to have proper "x.y" values Aug 19, 2019
@@ -29,6 +29,21 @@ namespace :test do
end
end

task :deps do

Choose a reason for hiding this comment

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

Would be nice to add a desc string to explain what this task does and also to have it appear in the rake -T task list

@@ -29,6 +29,21 @@ namespace :test do
end
end

task :deps do
src = `curl -sL https://github.com/inspec/inspec-aws/archive/master.tar.gz | tar xO inspec-aws-master/Gemfile`

Choose a reason for hiding this comment

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

This task isn't going to be usable by Windows users. Since the task is informational and not critical to using the plugin, that's probably OK.

# spec.add_dependency "aws-sdk-appstream", "~> 1"
# spec.add_dependency "aws-sdk-appsync", "~> 1"
spec.add_dependency "aws-sdk-athena", "~> 1"
# spec.add_dependency "aws-sdk-acm", "~> 1.0"

Choose a reason for hiding this comment

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

👍 to these dep changes

@james-stocks james-stocks requested a review from a team December 10, 2019 08:57
@james-stocks
Copy link

Dep changes look good to me but I'm not familiar with the AWS gems so adding @inspec/inspec-cloud-devs as a reviewer. While this change is a valid correction, I don't know what other projects might be depending on a version being >= 2.0

@zenspider zenspider force-pushed the zenspider/more-dep-stuff branch from 6c1fea5 to 629b650 Compare December 10, 2019 20:09
I don't think it works the way they think it works...

Signed-off-by: Ryan Davis <[email protected]>
Prints out missing and (possibly) extra dependencies.

Signed-off-by: Ryan Davis <[email protected]>
@zenspider zenspider force-pushed the zenspider/more-dep-stuff branch from 629b650 to 9d15496 Compare December 10, 2019 20:15
@zenspider zenspider merged commit 2af4750 into master Dec 10, 2019
@chef-expeditor chef-expeditor bot deleted the zenspider/more-dep-stuff branch December 10, 2019 21:14
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.

3 participants