-
Notifications
You must be signed in to change notification settings - Fork 314
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
rename cookbook source/sources to dependency/dependencies #640
Conversation
@ivey @andrewGarson @sethvargo check this out |
First thought: can we merge other PRs before touching this? There are so PRs that fix critic issues that likely stomp on this and will make for some ugly merge conflicts. |
@sources.delete(source.to_s) | ||
# @return [Berkshelf::Dependency] | ||
def remove_dependency(dependency) | ||
@dependenciesz.delete(dependency.to_s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo?
After review thought: we should either:
This could get messy if we are adding these features in 2.0 when we really need to get 2.0 stable and then dedicate work to 3.0. Just my 0.02:dollar: |
@sethvargo there is a branch We are going to be aggressively developing towards 3.0 and I'd rather not have 2.0 be the focus, so I think keeping 3.0.0.dev in the master branch is the right approach. We should point out the pull requests that are crucial for 2-0-stable and make sure they merge cleanly into that. Git merging is good enough that we should be able to merge those into master even if this makes it in first. |
@reset right. I guess what I'm saying is that we have some serious bugs that are squashed in existing PRs. 2.0 has a lot of bugs, and I'd like to get most of those issues cleared up before we move onto the dependency API stuff |
@sethvargo I understand. I'll take care of any merges into master, don't worry about it. Clean up whatever PR's you have that address bugs and we can try to merge them in tomorrow. |
Well, I can't figure out the strange failures on 1.9 and/or the strange celluloid timeouts that randomly happen |
@sethvargo I'll look at each one of them tomorrow and run them locally on my machine to verify. We can figure out what is causing the failures on Travis in another PR. |
rename add_source_dependencies to add_recursive_dependencies
lockfile. The lockfile still contains an entry called "sources". We will need to migrate old lockfiles again to update these lockfiles to the proper terminology
@sethvargo all of the PRs you were worried about are merged into master and master is rebased against this branch. All of your inline comments have been fixed as well. @ivey @sethvargo please give this a look over again |
" In Berksfile.lock:\n" + | ||
" #{@locked_dependency.name} (#{@locked_dependency.locked_version})\n\n" + | ||
"Try running `berks update #{@dependency.name}, which will try to find '#{@dependency.name}' matching " + | ||
"'#{@dependency.version_constraint}'." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reset the syntax was messed up here. Not sure if this was because of another pull in or if you changed it on purpose. I personally find it much more difficult to read with the extra newlines converted to \n
s. I'm very partial to the [].join("\n")
syntax for error messages because they appear more like the actual error will read.
subject.remove_dependency(dependency) | ||
expect(subject.dependencies).to have(0).items | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄
👍 |
rename cookbook source/sources to dependency/dependencies
This is in preparation of removing the
chef_api
andsite
location keys from the Berksfile and replacing them withsource
. Unfortunately we called cookbook dependencies "sources" early on and it stuck. This will rename them to be what they actually are: dependencies.