project overrides of software definitions #94

Merged
merged 10 commits into from Feb 27, 2014

Projects

None yet

6 participants

@lamont-granquist
Collaborator
  • allows overriding version and source from the project file
  • fixes overriding version from distro overrides
@lamont-granquist
Collaborator

@schisamo @sersut @danielsdeleo want to get feedback on this WIP before i get too far.

my chefdk project file right now looks like this:

name "chefdk"
maintainer "Opscode, Inc."
homepage "http://www.opscode.com"

install_path    "/opt/chefdk"
build_version   Omnibus::BuildVersion.full
build_iteration 4

overrides( {
  "ruby" => {
    "version" => "2.1.0",
    "source" => {
      :url => "http://ftp.ruby-lang.org/pub/ruby/2.1/ruby-2.1.0.tar.gz",
      :md5 => '9e6386d53f5200a3e7069107405b93f7',
    },
  },
} )

dependency "preparation"
dependency "chefdk"
dependency "ohai" if ENV["OHAI_GIT_REV"]
dependency "version-manifest"

wanted to ask about the format of the overrides line there since the bigass-hash we're going to get there is a bit fugly. i also don't know how far down the road i want to get in re-implementing Chef::Node::Attribute for omnibus...

@danielsdeleo
Member

Maybe this?

override("ruby") do
  version "2.1.0"
  source :url => "http://ftp.ruby-lang.org/pub/ruby/2.1/ruby-2.1.0.tar.gz",
      :md5 => '9e6386d53f5200a3e7069107405b93f7'

  # SEE NOTE BELOW
  relative_path "ruby-#{version}"
end

You could accomplish that just by doing an instance_eval with the block. The downside is that any place you create a string like "foo-#{version}" you have to be able to fix also. In the case of the ruby software definition, this is just the relative_path setting.

@lamont-granquist
Collaborator

yeah, the problem is that if the software definition interpolates the version into the build block then you wind up needing to duplicate the build block, which starts to get close to needing to re-write the software definition inside the project overrides.

i'd prefer to instance_eval the overrides first before eval'ing the project, but then i need to have the 'setter' methods save overrides when i'm doing that, and then the 'getters' need to return them if they're set... internally i can have two hashes for variables (both looking like the overrides variable i have now), but it'd be better to make the instance variables be objects themselves that had default and override attributes in them... hence, not sure how far down that road to go...

@danielsdeleo
Member

Fair point. I guess you could just say "YAGNI" and leave that problem for later, or you could instance eval the overrides first and change the DSL setters to ||= behavior, or you could update the DSL methods to accept blocks and then lazy eval them. I guess I'm just kinda adverse to, as you say, "re-implementing Chef::Node::Attribute" since it ends up being super complicated.

@sersut
Member
sersut commented Feb 19, 2014

I thought this was the design we came up with in the other discussion:

# in omnibus-software/config/software/ruby.rb
name 'ruby'
default_version '1.9.3' 

version '1.9.3' do
  source 'http://ftp.ruby-lang.org/pub/ruby/1.9/ruby-1.9.3-p484.tar.gz'
  md5 '8ac0dee72fe12d75c8b2d0ef5d0c2968'
end

version '2.1.0' do
  source 'http://ftp.ruby-lang.org/pub/ruby/2.1/ruby-2.1.0.tar.gz'
  md5 '9e6386d53f5200a3e7069107405b93f7'
end
# in omnibus-chef/config/projects/chef-dk.rb
override 'ruby', version: '2.1.0'
override 'foo', version: 'bar'

Did we decide to deviate from that?

@lamont-granquist
Collaborator

okay @sersut that's actually closer to what I've got now, only with ruby 1.9 hash syntax and not ruby 1.8 hash syntax. that is actually different from what dan is suggesting and from the road it looks like you were going down...

what i wrote is equivalent to this:

overrides( {
  ruby: {
    version: "2.1.0",
    source: {
      url: "http://ftp.ruby-lang.org/pub/ruby/2.1/ruby-2.1.0.tar.gz",
      md5: '9e6386d53f5200a3e7069107405b93f7',
    },
  },
} )

i can get this easily just by tweaking the project DSL and writing nicer hash syntax:

overrides 'ruby',
    version: "2.1.0",
    source: {
      url: "http://ftp.ruby-lang.org/pub/ruby/2.1/ruby-2.1.0.tar.gz",
      md5: '9e6386d53f5200a3e7069107405b93f7',
    }

but that's still passing a hash, and not eval'ing from the software DSL like dan was going...

@schisamo
Member

@lamont-granquist I see the deviations @sersut speaks of. A couple of thoughts:

  • The changes to the Software DSL that we all spiked on had the idea of declaring a known universe of possible versions. That's what the first omnibus-software/config/software/ruby.rb snippet @sersut shared above does. Once nice thing about this approach is the extended attributes/data about a particular version live with the software definition and can be shared by multiple projects.
  • The Project DSL change was an additive attribute named override (vs the batch overrides) that most likely appends to an internal @overrides hash. This matches up with the 1.0.0+ change of preferring depends instead of dependencies. I personally think this makes things more readable/explicit, easier to validate and depends less on magic hash structures.

I would prefer we stick to a design once we agree upon it as a group.

@sersut
Member
sersut commented Feb 19, 2014

Also overriding the source at the project level means that every project that wants to use Ruby 2.1.0 should code the override for source and md5. Having those inside the software definition I think simplifies things from user point of view.

@lamont-granquist
Collaborator

@sersut @schisamo @danielsdeleo okay here ya go. the patch now makes software definitions work like the following. you can either pass version a block or else just do interpolation on version. fixed the repo-level version overrides so that they work now as well.

name "zlib"
default_version "1.2.6"

dependency "libgcc"

version "1.2.6" do
  source md5: "618e944d7c7cd6521551e30b32322f4a"
end

version "1.2.8" do
  source md5: "44d667c142d7cda120332623eab69f40"
end

source :url => "http://downloads.sourceforge.net/project/libpng/zlib/#{version}/zlib-#{version}.tar.gz"
@lamont-granquist
Collaborator

The project DSL to do overrides looks like this:

name       "chefdk"
maintainer "Opscode, Inc."
homepage   "http://www.opscode.com"

install_path    "/opt/chefdk"
build_version   Omnibus::BuildVersion.full
build_iteration 4

override :berkshelf, version: "3.0.0.beta6"
override :bundler,   version: "1.5.2"
override :libedit,   version: "20130712-3.1"
override :libtool,   version: "2.4.2"
override :libxml2,   version: "2.9.1"
override :libxslt,   version: "1.1.28"
override :nokogiri,  version: "1.6.1"
override :ruby,      version: "2.1.0"
override :rubygems,  version: "2.2.1"
override :yajl,      version: "1.2.0"
override :zlib,      version: "1.2.8"

dependency "preparation"
dependency "chefdk"
dependency "ohai" if ENV["OHAI_GIT_REV"]
dependency "version-manifest"
@sersut sersut and 1 other commented on an outdated diff Feb 24, 2014
lib/omnibus/software.rb
@@ -143,16 +177,37 @@ def dependencies(val=NULL_ARG)
# if more than one pair is given, or if no source value is ever
# set.
def source(val=NULL_ARG)
- @source = val unless val.equal?(NULL_ARG)
- @source
+ unless val.equal?(NULL_ARG)
+ @source ||= {}
+ @source.merge!(val)
+ end
+ apply_overrides(:source) || raise(MissingSoftwareConfiguration.new("source", "[Hash]"))
@sersut
sersut Feb 24, 2014 Chef Software, Inc. member

Looks like currently we can only override version and source.

Should we do some meta-programming magic so that this is done for all DSL attributes @schisamo ?

@lamont-granquist
lamont-granquist Feb 24, 2014 collaborator

that is what is hard. since you're doing method dispatch, you wind up really wanting to instance_eval a block, but then you have to be able to set default and override attrs based on weither you're in the context of the project opverrides or the software defn, and that is the problem that winds up reengineering a version of Chef::Node::Attribute in order to deal with it properly. at that point I'd probably go with Dan's suggestion and re-engineer to project DSL to take a block instead of a symbol+hash as well.

so this is exactly why i was asking all the questions again... arguably that's the more powerful way to extend the DSL, but i think YAGNI...

@lamont-granquist
lamont-granquist Feb 24, 2014 collaborator

the other thing is that with pushing all the code into the software definition and wrapping it with version "x.y.z" do .. end blocks then we shouldn't ever need anything other than version to be overridden in the project. i had source initially so i just left it in since it didn't seem too harmful... i would actually go the other direction, though, and rip out source and force pushing the version dependencies into the software defn.

@sersut
sersut Feb 24, 2014 Chef Software, Inc. member

Agreed YAGNI... 👍

@sersut
Member
sersut commented Feb 24, 2014

Overall this looks good to me @lamont-granquist.

One thing worths discussing IMO is that, do we want to retain the repo_overrides functionality after having project level overrides? @schisamo and I chatted about the fact that there is no known project that uses this functionality right now and it's making things slightly complicated IMO.

Maybe we can remove it and make a version bump since Dan has the pkg package creation that requires version bump anyways? @schisamo thoughts on that?

@lamont-granquist
Collaborator

repo_overrides were fundamentally broken, so i think we can safely rip them out without too much ceremony.

lamont-granquist added some commits Feb 24, 2014
@lamont-granquist lamont-granquist add name of software that throws exception for debugging 3704855
@lamont-granquist lamont-granquist raising here doesn't work
- we could add a source? method and use that to inspect if there
  is a source and make #source throw an exception but right now
  having an unset source is perfectly acceptable for gem installs
bee9f04
@sdelano
Collaborator
sdelano commented Feb 25, 2014

What are repo_overrides?

@lamont-granquist
Collaborator

that's the omnibus.overrides file in the root of the repo that lets you add config like the gitsha of chef to build.

@sdelano
Collaborator
sdelano commented Feb 25, 2014

@lamont-granquist Cool, thanks! Just wanted to make sure it wasn't something that we were using on the server.

@schisamo schisamo and 2 others commented on an outdated diff Feb 25, 2014
lib/omnibus/software.rb
@@ -49,18 +50,28 @@ class Software
attr_reader :project
- attr_reader :given_version
- attr_reader :override_version
+ attr_reader :version
+
+ def given_version # deprecated
+ @version
@schisamo
schisamo Feb 25, 2014 Chef Software, Inc. member

We should print a message for anything marked deprecated. Let's also add an @deprecated YARD comment.

@sethvargo
sethvargo Feb 25, 2014 Chef Software, Inc. member

And a @todo remove in x.y

@lamont-granquist
lamont-granquist Feb 25, 2014 collaborator

I started using those in the log message in omnibus/builder.rb, though, so i might just remove the deprecated comment instead and document them...

@sethvargo
sethvargo Feb 25, 2014 Chef Software, Inc. member

@lamont-granquist the advantage to using YARD here is that we can have a build step to remind us to remove them. It's also nice to generate a list of TODOs programatically.

@lamont-granquist
lamont-granquist Feb 25, 2014 collaborator

yeah, but #given_version is actually useful and you can't get at that from the #version accessor, so without more work reshaping the API, i think my #deprecated comment is just wrong now... so yes YARD, but to document that as an accessor, maybe with a @todo to do something more generic in the future for accessing overridden variables... the backlog of @todos in omnibus-ruby is kind of huge though...

@sethvargo
sethvargo Feb 25, 2014 Chef Software, Inc. member

@lamont-granquist be careful using @todo without backticks. You just tagged someone 😄. But I see what you mean

@lamont-granquist
lamont-granquist Feb 25, 2014 collaborator

:lol: i'm getting hypoglycemic need to find some lunch... looks like i tagged two different users as well...

@sethvargo
sethvargo Feb 25, 2014 Chef Software, Inc. member

☕️

@schisamo
Member

@lamont-granquist @sersut We should just remove repo_overrides. Let's also add some entries in the CHANGELOG for this PR. Designing on the fly here, but maybe under a new section of ## UNRELEASED VERSION. This matches the approach we are beginning to use for other projects.

@sethvargo sethvargo and 1 other commented on an outdated diff Feb 25, 2014
lib/omnibus/builder.rb
@@ -210,6 +210,7 @@ def log(message)
def build
log "building #{name}"
+ log "version overridden from #{@software.given_version} to #{@software.override_version}" if @software.overridden?
@sethvargo
sethvargo Feb 25, 2014 Chef Software, Inc. member

It seems a little strange that we adhere to the 80-character line limit for things like @build_commands <<, but not this log message...

@lamont-granquist
lamont-granquist Feb 25, 2014 collaborator

up for suggestions on how to break this that doesn't read worse

@sethvargo
sethvargo Feb 25, 2014 Chef Software, Inc. member

@lamont-granquist

log "version overridden from #{@software.given_version} to" \
    "#{@software.override_version}" if @software.overridden?
@sethvargo sethvargo commented on an outdated diff Feb 25, 2014
lib/omnibus/software.rb
@@ -49,18 +50,28 @@ class Software
attr_reader :project
- attr_reader :given_version
- attr_reader :override_version
+ attr_reader :version
+
+ def given_version # deprecated
+ @version
+ end
+
+ attr_reader :overrides
+
+ def override_version # deprecated
@sethvargo
sethvargo Feb 25, 2014 Chef Software, Inc. member

We should use @deprecated and an @todo remove in x.y

@sethvargo sethvargo commented on an outdated diff Feb 25, 2014
lib/omnibus/software.rb
@@ -130,6 +151,19 @@ def dependencies(val=NULL_ARG)
@dependencies
end
+ def apply_overrides(attr)
+ # convert :foo into :@foo for instance_variable_get
+ instance_symbol = attr.to_s.sub(/^/, '@').to_sym
+ val = instance_variable_get(instance_symbol)
@sethvargo
sethvargo Feb 25, 2014 Chef Software, Inc. member

This logic seems a bit complex. I like that there's a comment, but the general pattern I see in other Ruby libs is:

val = instance_variable_get(:"@{attr}")
@sethvargo sethvargo commented on an outdated diff Feb 25, 2014
spec/data/projects/chefdk.rb
+
+override :berkshelf, version: "3.0.0.beta6"
+override :bundler, version: "1.5.2"
+override :libedit, version: "20130712-3.1"
+override :libtool, version: "2.4.2"
+override :libxml2, version: "2.9.1"
+override :libxslt, version: "1.1.28"
+override :nokogiri, version: "1.6.1"
+override :ruby, version: "2.1.0"
+override :rubygems, version: "2.2.1"
+override :yajl, version: "1.2.0"
+override :zlib, version: "1.2.8"
+
+dependency "preparation"
+dependency "chefdk"
+dependency "ohai" if ENV["OHAI_GIT_REV"]
@sethvargo
sethvargo Feb 25, 2014 Chef Software, Inc. member

This seems magical and undocumented

@sethvargo sethvargo commented on an outdated diff Feb 25, 2014
spec/data/projects/chefdk.rb
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+name "chefdk"
+maintainer "Opscode, Inc."
+homepage "http://www.opscode.com"
@sethvargo
sethvargo Feb 25, 2014 Chef Software, Inc. member

getchef.com

@sethvargo sethvargo commented on an outdated diff Feb 25, 2014
spec/data/projects/chefdk.rb
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+name "chefdk"
+maintainer "Opscode, Inc."
@sethvargo
sethvargo Feb 25, 2014 Chef Software, Inc. member

Chef Software

@sethvargo sethvargo commented on an outdated diff Feb 25, 2014
spec/data/software/zlib.rb
@@ -0,0 +1,70 @@
+#
+# Copyright:: Copyright (c) 2012 Opscode, Inc.
@sethvargo
sethvargo Feb 25, 2014 Chef Software, Inc. member

Needs updated

@sethvargo sethvargo commented on an outdated diff Feb 25, 2014
lib/omnibus/software.rb
def source(val=NULL_ARG)
- @source = val unless val.equal?(NULL_ARG)
- @source
+ unless val.equal?(NULL_ARG)
+ @source ||= {}
+ @source.merge!(val)
+ end
+ apply_overrides(:source)
+ end
+
+ # Set or retrieve the default version of the software
+ def default_version(val=NULL_ARG)
+ @version = val unless val.equal?(NULL_ARG)
@sethvargo
sethvargo Feb 25, 2014 Chef Software, Inc. member

Need to return @version here

@sethvargo sethvargo commented on an outdated diff Feb 25, 2014
lib/omnibus/software.rb
@@ -169,7 +223,8 @@ def whitelist_file(file)
#
# @return [Boolean]
def overridden?
- @override_version && (@override_version != @given_version)
+ # note: we hit the instance variables and bypass the accessors very deliberately
@sethvargo
sethvargo Feb 25, 2014 Chef Software, Inc. member

I like the comment, but why? (I know the answer, but a future developer may not. It would be helpful to explain why using the accessors is a bad idea.)

@sethvargo sethvargo commented on an outdated diff Feb 25, 2014
spec/data/projects/chefdk.rb
@@ -0,0 +1,41 @@
+#
+# Copyright:: Copyright (c) 2012 Opscode, Inc.
@sethvargo
sethvargo Feb 25, 2014 Chef Software, Inc. member

Needs updated

@sethvargo sethvargo commented on an outdated diff Feb 25, 2014
spec/data/software/zlib.rb
+#
+
+name "zlib"
+default_version "1.2.6"
+
+dependency "libgcc"
+
+version "1.2.6" do
+ source md5: "618e944d7c7cd6521551e30b32322f4a"
+end
+
+version "1.2.8" do
+ source md5: "44d667c142d7cda120332623eab69f40"
+end
+
+# TODO: this link is subject to change with each new release of zlib.
@sethvargo
sethvargo Feb 25, 2014 Chef Software, Inc. member

Is this TODO still valid? It seems like the URL given is calculated off the version.

@lamont-granquist
Collaborator

okay @sethvargo pushed a bunch of updates ^^^

@sethvargo sethvargo commented on the diff Feb 27, 2014
lib/omnibus/project.rb
@@ -124,7 +125,7 @@ def package_name(val=NULL_ARG)
# must be set in order to build a project)
def install_path(val=NULL_ARG)
@install_path = val unless val.equal?(NULL_ARG)
- @install_path || raise(MissingProjectConfiguration.new("install_path", "/opt/opscode"))
+ @install_path || raise(MissingProjectConfiguration.new("install_path", "/opt/chef"))
@sethvargo
sethvargo Feb 27, 2014 Chef Software, Inc. member

Is this a find/replace error, or are we actually changing this path?

@lamont-granquist
lamont-granquist Feb 27, 2014 collaborator

its an example in an error message, not a find/replace bug

@sethvargo
sethvargo Feb 27, 2014 Chef Software, Inc. member

Right, but does the project live at /opt/chef yet?

@lamont-granquist
lamont-granquist Feb 27, 2014 collaborator

its just an example, if you don't have install_path it renders an exception with a path in there -- it could be "/opt/path/to/your/software". unless you feed omnibus a busted project with no install_path its not an executed code path...

@sethvargo
sethvargo Feb 27, 2014 Chef Software, Inc. member

Ahh okay. Sorry. ☕️

@schisamo schisamo referenced this pull request Feb 27, 2014
Merged

Release Engineerify #96

@sethvargo
Member

👍

@schisamo
Member

@lamont-granquist This looks great! :shipit: 🚢

@lamont-granquist lamont-granquist merged commit 7d3ad8b into master Feb 27, 2014

1 check passed

Details default The Travis CI build passed
@sersut
Member
sersut commented Feb 27, 2014

woot!

@sethvargo sethvargo deleted the versioned-dependencies-2 branch Feb 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment