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

(CODEMGMT-1415) SPIKE assume modules unchanged locally #1145

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/r10k/action/deploy/environment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ def visit_environment(environment)
started_at = Time.new
@environment_ok = true

environment.read_previous_puppetfile if @assume_unchanged

status = environment.status
logger.info _("Deploying environment %{env_path}") % {env_path: environment.path}

Expand Down Expand Up @@ -186,6 +188,7 @@ def allowed_initialize_opts
super.merge(puppetfile: :self,
cachedir: :self,
'no-force': :self,
'assume-unchanged': :self,
'generate-types': :self,
'puppet-path': :self,
'puppet-conf': :self,
Expand Down
1 change: 1 addition & 0 deletions lib/r10k/cli/deploy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ def self.command
DESCRIPTION

flag :p, :puppetfile, 'Deploy modules from a puppetfile'
flag nil, :'assume-unchanged', 'Assume previously deployed modules have not changed'
option nil, :'default-branch-override', 'Specify a branchname to override the default branch in the puppetfile',
argument: :required

Expand Down
17 changes: 17 additions & 0 deletions lib/r10k/environment/base.rb
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
require 'r10k/logging'
require 'r10k/util/subprocess'
require 'r10k/stub_puppetfile'

# This class defines a common interface for environment implementations.
#
# @since 1.3.0
class R10K::Environment::Base

include R10K::Logging

# @!attribute [r] name
# @return [String] A name for this environment that is unique to the given source
attr_reader :name
Expand Down Expand Up @@ -48,10 +52,23 @@ def initialize(name, basedir, dirname, options = {})
@full_path = File.join(@basedir, @dirname)
@path = Pathname.new(File.join(@basedir, @dirname))


@puppetfile = R10K::Puppetfile.new(@full_path, nil, nil, @puppetfile_name)
@puppetfile.environment = self
end

def read_previous_puppetfile
previous_puppetfile = nil
if File.exist?(File.join(@full_path, @puppetfile_name || 'Puppetfile'))
previous_puppetfile = R10K::StubPuppetfile.new(@full_path, nil, nil, @puppetfile_name)
else
logger.debug _("Could not find Puppetfile at: %{path}" % { path: File.join(@full_path, @puppetfile_name || 'Puppetfile') })
end

previous_puppetfile.load if previous_puppetfile
@puppetfile.previous_version = previous_puppetfile
end

# Synchronize the given environment.
#
# @api public
Expand Down
3 changes: 0 additions & 3 deletions lib/r10k/environment/git.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
require 'r10k/logging'
require 'r10k/puppetfile'
require 'r10k/git/stateful_repository'
require 'forwardable'
Expand All @@ -8,8 +7,6 @@
# @since 1.3.0
class R10K::Environment::Git < R10K::Environment::WithModules

include R10K::Logging

R10K::Environment.register(:git, self)
# Register git as the default environment type
R10K::Environment.register(nil, self)
Expand Down
2 changes: 0 additions & 2 deletions lib/r10k/environment/svn.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
# @since 1.3.0
class R10K::Environment::SVN < R10K::Environment::Base

include R10K::Logging

R10K::Environment.register(:svn, self)

# @!attribute [r] remote
Expand Down
3 changes: 0 additions & 3 deletions lib/r10k/environment/with_modules.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
require 'r10k/logging'
require 'r10k/util/purgeable'

# This abstract base class implements an environment that can include module
Expand All @@ -7,8 +6,6 @@
# @since 3.4.0
class R10K::Environment::WithModules < R10K::Environment::Base

include R10K::Logging

# @!attribute [r] moduledir
# @return [String] The directory to install environment-defined modules
# into (default: #{basedir}/modules)
Expand Down
65 changes: 65 additions & 0 deletions lib/r10k/mock_module.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
require 'puppet_forge'

class R10K::MockModule

# @!attribute [r] title
# @return [String] The forward slash separated owner and name of the module
attr_reader :title

# @!attribute [r] name
# @return [String] The name of the module
attr_reader :name

# @param [r] dirname
# @return [String] The name of the directory containing this module
attr_reader :dirname

# @!attribute [r] owner
# @return [String, nil] The owner of the module if one is specified
attr_reader :owner

# @!attribute [r] path
# @return [Pathname] The full path of the module
attr_reader :path

# @!attribute [r] version
# @return [Pathname] The version, if it can be statically determined from args
attr_reader :version

# @param title [String]
# @param dirname [String]
# @param args [Array]
def initialize(title, dirname, args, environment=nil)
@title = PuppetForge::V3.normalize_name(title)
@dirname = dirname
@owner, @name = parse_title(@title)
@path = Pathname.new(File.join(@dirname, @name))
@version = find_version(args)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need all these fields? Is there some hidden contract that requires them?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need the version and name in the current implementation (which requires the title). I thought we might need the path but I don't think so. This file was just a copy and pared down version of the module base class.

end

private

def find_version(args)
if args.is_a?(String)
args
elsif args.is_a?(Hash)
if args[:type] == 'forge'
args[:version]
elsif args[:ref] && args[:ref].match(/[0-9a-f]{40}/)
args[:ref]
elsif args[:type] == 'git' && args[:version].match(/[0-9a-f]{40}/)
args[:version]
else
args[:tag] || args[:commit]
end
end
end

def parse_title(title)
if (match = title.match(/\A(\w+)\Z/))
[nil, match[1]]
elsif (match = title.match(/\A(\w+)[-\/](\w+)\Z/))
[match[1], match[2]]
end
end
end
22 changes: 21 additions & 1 deletion lib/r10k/puppetfile.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ class Puppetfile
# @return [String] The path to the Puppetfile
attr_reader :puppetfile_path

# @!attrbute [rw] previous_version
# @return [R10K::StubPuppetfile] A simplified version of this Puppetfile
# available on disk prior to the environment sync
attr_accessor :previous_version

# @!attribute [rw] environment
# @return [R10K::Environment] Optional R10K::Environment that this Puppetfile belongs to.
attr_accessor :environment
Expand Down Expand Up @@ -130,6 +135,19 @@ def add_module(name, args)
args[:default_branch_override] = @default_branch_override
end

no_change = false
if @previous_version
stub_module = R10K::MockModule.new(name, install_path, args)
logger.debug2 _("Checking previous Puppetfile for version %{version} of %{name}" % { version: stub_module.version, name: stub_module.name })
if stub_module.version == @previous_version.modules[stub_module.name].version
logger.debug _("Expected version has not changed between this and previous deployment, skipping %{name}" % { name: stub_module.name })
# The version in this Puppetfile is the same as the version previous
# specified in the Puppetfile (or we cannot be sure the versions
# specified are static, eg a branch specifier).
no_change = true
end
end

mod = R10K::Module.new(name, install_path, args, @environment)
mod.origin = :puppetfile

Expand All @@ -144,7 +162,9 @@ def add_module(name, args)
@managed_content[install_path] = Array.new unless @managed_content.has_key?(install_path)
@managed_content[install_path] << mod.name

@modules << mod
# We want to manage the content (and not purge it) even if we do not want
# to sync modules that we assume unchanged.
no_change ? @modules : @modules << mod
Copy link
Contributor

@reidmv reidmv Apr 19, 2021

Choose a reason for hiding this comment

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

I worry about focusing effort on the Puppetfile, when I see the future direction being more "modules attached to an environment", with the Puppetfile being kind of a legacy way to attach them.

Is it possible to take something like this approach instead?

  1. Add something like #insync? and #insync= methods to module objects. Also #insync to get the detailed value.
  2. In the Puppetfile, set mod.insync = true/false/:unknown as appropriate. When :unknown, #insync? returns false.
  3. Instead of adding logic to mask the existence of the module here in the Puppetfile code, figure out where the module is actually synced/verified, and put an ... unless mod.insync? there. I think that might be in the environment deploy action (to pass e.g. an @verify flag) and/or in the module sync method.

That approach would also preserve the ability to "see" all modules attached to an environment, regardless of what you're planning on doing to/with them.

Again, mostly I'm just worried about embedding important logic in the Puppetfile specifically, given that the Puppetfile is only one way to define environment content. At some point a general encapsulation/cleanup of how environments and modules relate would be nice, but in the meantime I think not implementing new stuff as Puppetfile-specific would be a good idea.

end

include R10K::Util::Purgeable
Expand Down
Loading