Tarball #1159

reidmv · 2021-05-06T21:52:38Z

This PR implements:

An R10K::Tarball base class
An R10K::Module::Tarball class, providing a "tarball" module type
An R10K::Environment::Tarball class, providing a "tarball" environment type

Example usage:

---
production:
  type: tarball
  source: https://example.com/puppet-code-env-v1.0.0.tar.gz
  version: 99a906c99c2f144de43f2ae500509a7474ed11c583fb623efa8e5b377a3157f0 # sha256digest
  modules:
    puppetlabs-peadm:
      type: forge
      version: 2.5.0
    reidmv-xampl:
      type: git
      source: https://github.com/reidmv/reidmv-xampl.git
      version: 62d07f2
    reidmv-mymodule:
      type: tarball
      source: https://example.com/reidmv-mymodule-1.0.0.tar.gz
      version: 6128ada158622cd90f8e1360fb7c2c3830a812d1ec26ddf0db7eb16d61b7293f # sha256digest

Later enhancements built on top of this might include Artifactory types, or S3 types, with options for real versions, depending on how the remote repository works.

Solves #567

This class will serve as a base class for creating tarball environment and module types.

Trying to make sure the global cachedir option gets respected by tarball

It's not just a development dependency; it's an r10k dependency too. Really it always has been because it's a puppetforge dependency.

This permits modules of type tarball to be used. Tarball modules will be extracted with no modification into the modules target directory. Tarballs should not be packed to include a top-level containing folder. modules: lwf-remote_file: type: tarball source: https://forge.puppet.com/v3/files/lwf-remote_file-1.1.3.tar.gz version: bce01b849631926b452900eff5dc385c893c4320c95b290f992e3c529f417f0e

This permits core environment content to be sourced from tarballs. The tarball environment type supports defining modules as well. --- production: type: tarball source: https://example.com/puppet-code-env-1.0.0.tar.gz version: 99a906c99c2f144de43f2ae500509a7474ed11c583fb623efa8e5b377a3157f0

These aren't fully featured tests, but it at least gets the skeleton up and structured.

I'm honestly not sure what this does, but it seems to be the cool new thing; all the other modules have it.

lib/r10k/tarball.rb

mwaggett · 2021-08-30T18:07:14Z

lib/r10k/tarball.rb

+
+    # @!attribute [r] sha256digest
+    #   @return [String] The tarball's expected sha256 digest
+    attr_accessor :sha256digest


Can we just call this sha256?

Honestly, I'm leaning towards version being the term that gets documented.

If we shorten it, I think digest (or the commonly used but slightly more technically ambiguous checksum) would be the preferred shortening. That's the part that conveys what this parameter is really about—the output of a hash function which can be used to validate the expected content.

I went with the very specific and technical sha256digest because I was trying to avoid the user needing to look up which algorithm to use for the checksum. However, we could probably expose that later by offering a checksum_algorithm if a use case arose for it.

I've put in a commit to change this to just checksum for now.

mwaggett · 2021-08-30T18:22:25Z

lib/r10k/tarball.rb

+    #        tarball content
+    #
+    # @return [Boolean]
+    def insync?(target_dir, purge: true)


Can you add a yardoc for the purge param? It's not super clear to me what it's being used for.

Renamed the parameter to ignore_untracked_files and added a yarddoc.

mwaggett · 2021-08-30T18:22:58Z

lib/r10k/tarball.rb

+    #        tarball content
+    #
+    # @return [Boolean]
+    def insync?(target_dir, purge: true)


Suggested change

def insync?(target_dir, purge: true)

def in_sync?(target_dir, purge: true)

don't love smushing words together 😅

Yeah. I tend to agree, but insync as a term seems to be the r10k convention. As such, I think it's maybe best to leave it this way in service to the greater goal of consistency.

mwaggett · 2021-08-30T18:27:40Z

lib/r10k/tarball.rb

+    #
+    # @return [Boolean]
+    def insync?(target_dir, purge: true)
+      target_dir_entries = Find.find(target_dir).map(&:to_s) - [target_dir]


Looking at https://ruby-doc.org/stdlib-2.6.3/libdoc/find/rdoc/Find.html, I don't think this is giving you a collection of entries. You might just want Dir[File.join(target_dir, '**', '*')]?

I took a look into this, and while I'm not 100% sure why it works this way, it seems that Find.find() returns an iterator that will hit all the files it is supposed to, and if that iterator is passed to #map, it does what's expected. The end result is an array of strings.

The globbing behavior is actually not complete, as it misses dot-files. Given that Find.find is working, I think it's best to leave it. I suspect it's more reliable than globbing can be.

mwaggett · 2021-08-30T18:51:37Z

lib/r10k/tarball.rb

+        tempfile.close
+        tempfile.unlink


Do these need to be in an ensure?

Yep, that would be preferred. Pushed a commit to address this by switching to Tempfile::open with a block.

mwaggett · 2021-08-30T18:55:18Z

lib/r10k/tarball.rb

+      unless sha256digest.nil?
+        File.exist?(cache_path) && sha256digest == file_digest(cache_path)
+      else
+        File.exist?(cache_path)
+      end


I would switch these around and use an if instead of unless. Using unless with else is hard to read imo.

or you could just do

Suggested change

unless sha256digest.nil?

File.exist?(cache_path) && sha256digest == file_digest(cache_path)

else

File.exist?(cache_path)

end

return false unless File.exist?(cache_path)

return false if sha256digest && sha256digest != file_digest(cache_path)

return true

Good call. Adding a commit to simplify.

mwaggett · 2021-08-30T19:06:15Z

lib/r10k/tarball.rb

+    #
+    # @param reader [String] An object that responds to #read
+    # @return [String] The read data's sha256 hex digest
+    def reader_digest(reader)


Does it make sense to put these digest methods into some utility class that can be shared? They don't necessarily seem specific to this tarball feature.

Also I think they should include sha256 in the names, since digest is fairly general.

The names were originally left generic in order to be applicable in the event the digest algorithm ever changes, or is made selectable per-tarball. I've tried making that more evident by using a @checksum_algorithm instance variable, and references such as the following.

# At this time, the only checksum type supported is sha256. In the future, # we may decide to support other algorithms if a use case arises. TBD. @checksum_algorithm = :SHA256

def reader_digest(reader) digest = Digest(@checksum_algorithm)

mwaggett · 2021-08-30T19:07:37Z

lib/r10k/tarball.rb

+      digest.hexdigest
+    end
+
+    def sanitized_dirname(string)


Isn't this defined elsewhere? We should probably pull it into a place that can be shared.

Created an R10K::Util::Cacheable module to share this method.

lib/r10k/tarball.rb

mwaggett · 2021-08-30T19:14:48Z

lib/r10k/tarball.rb

+      string.gsub(/[^@\w\.-]/, '-')
+    end
+
+    def http_get(uri, method: Net::HTTP::Get, redirect_limit: 10, &block)


If you want this to be a generic http request method, I would change the method name to reflect that. Otherwise, if you want to keep it specific to GET, I would remove method as a param.

This also seems like something that should be shared, rather than specific to this class.

Removed method: as a user opt, and hard-coded to Net::HTTP::Get.

Regarding sharing, at the moment I don't know of anything else in R10k that needs to download files this way. I've added a comment to the method which clarifies exactly what it is doing:

# Start a Net::HTTP::Get connection, then yield the Net::HTTPSuccess object # to the caller's block. Follow redirects if Net::HTTPRedirection responses # are encountered, and honor settings[:proxy].

I think this could potentially be generalized for sharing, but I'm hesitant to break it off unless/until there's another consumer in r10k that wants to use it, for fear of over engineering. As near as I can tell R10K currently offloads all download-esque activity to other libraries. I'm considering Net::HTTP to be such a library, though, it's probably possible to use Faraday instead... I think we already have that as a dependency, if it gets us something Net::HTTP doesn't.

Reviewing Faraday, it looks like that library can do everything we need it to and more succinctly than Net::HTTP. Given that it's already an r10k dependency, the right thing to do may be to reduce this method to using Faraday instead.

I took a stab at using Faraday, but it looks like:

We want/need streaming downloads, so I think we need a newer version of Faraday than r10k currently supports, due to the puppet-forge gem dependency constraints.

Using the newer version of Faraday, we run into a bit of a problem for testing; I can't figure out how to make Faraday's test stubs work with streaming downloads.

If these two problems didn't exist, it seems like we could fairly handily offload this to Faraday. I'm stopping that line of investigation for now though. Seems pointless given item 1.

If it did work, it might look something like this.

# @param uri [URI] The URI to download from # @param output The file or path to save to # @return [String] The downloaded file's sha256 hex digest def download_to_file(uri, output) digest = DIGEST_CLASS.new File.open(output, 'wb') do |output_stream| faraday_connection.get(uri) do |req| req.options.on_data = Proc.new do |chunk, _| output_stream.write(chunk) digest.update(chunk) end end end digest.hexdigest end # Defined separately to enable test stubbing def faraday_connection Faraday.new do |faraday| faraday.response(:follow_redirects) faraday.adapter(:net_http) faraday.proxy(settings[:proxy]) unless settings[:proxy].nil? end end

Alright, last tweak for now: went ahead and moved all the file download/copy/checksum related methods out of R10K::Tarball and into a new utility module, R10K::Util::Downloader.

mwaggett · 2021-08-30T19:22:19Z

ugh not sure how my review comments got submitted before I was done.. More to come.. 😅

also @reidmv if/when you make updates, if you could make them in separate commits, that would be helpful for review (and then we can squash where relevant at the end).

Per PR review suggestion

Method used by both R10K::Git::Cache and R10K::Tarball

Co-authored-by: Molly Waggett <mwaggett@alumni.reed.edu>

We aren't doing anything besides Get, so just hard-code it for ease of understanding. The comment clarifies what this method does, and how it is used by consuming methods.

R10K::Tarball only supports sha256 at this time, but the door is being left open to potentially support other types of checksums in the future.

Co-authored-by: Molly Waggett <mwaggett@alumni.reed.edu>

Fix proxy and other setting propogation for R10K::Tarball. Use R10K::Util::Cacheable to centrally default a default cachedir setting. Can't set the default centrally yet because that might invalidate existing caches, may need to do it on a major version change.

For consistency, clarity

Taking methods originally defined in R10K::Tarball and placing them into a sharable utility module.

This completes a move of generalizable methods from R10K::Tarball into the sharable utility module R10K::Util::Downloader

Just name cache files by either their checksum (if known), or download URL (if no checksum given). While it's not gonna be super easy to eyeball the cache and know what is or isn't there, it better guarantees that the cache is fully efficient.

Previously, R10K::Module::Tarball#statically_defined_version only accepted the :version key. However, :checksum also constitutes a statically defined version, if it is passed. Thought: #statically_defined_version should be called after #initialize. Otherwise it's repeating work that's handled setting instance variables.

reidmv · 2021-09-17T16:59:48Z

@mwaggett ready for re-review! I solemnly promise to leave this alone and stop fiddling with it in my spare time until then.* 😇

* Unlikely to happen but if the mood strikes me I will not refrain from adding additional tests.

This commit implements tests for R10K::Util::Downloader#http_get, and fixes bugs found while doing that.

reidmv · 2021-10-04T18:01:04Z

I noticed that after the first review, this PR expanded from being a fairly new-files-only change into something that modifies how some existing structures work. @mwaggett if it's helpful for review and approval, I could probably break the PR apart into smaller requests, and split the structure changing stuff from the new feature stuff. Let me know.

reidmv · 2021-11-08T20:25:31Z

Closing in favor of #1244

reidmv force-pushed the tarball branch 4 times, most recently from be82a39 to ec69c47 Compare May 7, 2021 02:26

reidmv force-pushed the tarball branch from ec69c47 to eeb7f9f Compare June 17, 2021 18:52

reidmv mentioned this pull request Jun 17, 2021

(maint) Fix forge module implementation def bug #1169

Closed

mwaggett added the feature label Aug 5, 2021

reidmv force-pushed the tarball branch 6 times, most recently from 0fc7e24 to 622cece Compare August 27, 2021 20:51

reidmv marked this pull request as ready for review August 27, 2021 21:05

reidmv requested a review from a team August 27, 2021 21:05

reidmv force-pushed the tarball branch from 622cece to 25a5d66 Compare August 27, 2021 23:16

reidmv added 9 commits August 27, 2021 17:33

Initial tarball class

5a85436

This class will serve as a base class for creating tarball environment and module types.

Update initializers

63ceffb

Trying to make sure the global cachedir option gets respected by tarball

Update r10k.gemspec minitar dependency

1f98d03

It's not just a development dependency; it's an r10k dependency too. Really it always has been because it's a puppetforge dependency.

Add basic test stubs for tarball objects

735ef15

These aren't fully featured tests, but it at least gets the skeleton up and structured.

Make tarball comply with delete-spec-dir option

4f327b7

Make tarball work to copy files on Windows

4ad29ac

Add statically_defined_version for tarball module

a722b71

I'm honestly not sure what this does, but it seems to be the cool new thing; all the other modules have it.

reidmv force-pushed the tarball branch from 25a5d66 to a722b71 Compare August 28, 2021 00:33

mwaggett reviewed Aug 30, 2021

View reviewed changes

mwaggett self-assigned this Aug 30, 2021

mwaggett reviewed Aug 30, 2021

View reviewed changes

Use if instead of unless; simplify cache_valid?

354dd43

Per PR review suggestion

reidmv force-pushed the tarball branch 3 times, most recently from 110ba3f to b1e30bc Compare September 13, 2021 03:23

reidmv requested a review from mwaggett September 13, 2021 03:28

reidmv and others added 3 commits September 13, 2021 21:31

Centralize #sanitized_dirname method for DRY

c1c0919

Method used by both R10K::Git::Cache and R10K::Tarball

Improve error in lib/r10k/tarball.rb

fa9f735

Co-authored-by: Molly Waggett <mwaggett@alumni.reed.edu>

Remove opt, add comment to R10K::Tarball#http_get

57cb15f

We aren't doing anything besides Get, so just hard-code it for ease of understanding. The comment clarifies what this method does, and how it is used by consuming methods.

reidmv force-pushed the tarball branch from ed87a7b to cf1efbd Compare September 14, 2021 04:31

reidmv and others added 5 commits September 15, 2021 17:59

Clarify checksum selectivity decision

69f7849

R10K::Tarball only supports sha256 at this time, but the door is being left open to potentially support other types of checksums in the future.

Strengthen regexes on Tarball scheme selector

31f8b42

Update lib/r10k/tarball.rb

847dbe7

Co-authored-by: Molly Waggett <mwaggett@alumni.reed.edu>

Cache dir and method name changes in R10K::Tarball

4eda983

For consistency, clarity

reidmv force-pushed the tarball branch from 017f255 to 4eda983 Compare September 16, 2021 01:01

reidmv added 4 commits September 16, 2021 16:03

Implmement R10K::Util::Downloader

3913582

Taking methods originally defined in R10K::Tarball and placing them into a sharable utility module.

Make R10K::Tarball include R10K::Util::Downloader

7e7a199

This completes a move of generalizable methods from R10K::Tarball into the sharable utility module R10K::Util::Downloader

Simplify cachedir key scheme

ba4acc2

Just name cache files by either their checksum (if known), or download URL (if no checksum given). While it's not gonna be super easy to eyeball the cache and know what is or isn't there, it better guarantees that the cache is fully efficient.

Implement test for R10K::Util::Downloader#http_get

4e751ce

This commit implements tests for R10K::Util::Downloader#http_get, and fixes bugs found while doing that.

reidmv force-pushed the tarball branch from cc82506 to 4e751ce Compare September 19, 2021 23:29

reidmv mentioned this pull request Sep 24, 2021

Purge should purge directories too, not just files #1222

Merged

mwaggett added the awaiting codeowner action label Sep 25, 2021

This was referenced Nov 8, 2021

Implement R10K::Util::Cacheable #1242

Merged

Implmement R10K::Util::Downloader #1243

Merged

Tarball #1244

Merged

reidmv closed this Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tarball #1159

Tarball #1159

reidmv commented May 6, 2021 •

edited

Loading

mwaggett Aug 30, 2021

reidmv Sep 13, 2021

mwaggett Aug 30, 2021

reidmv Sep 13, 2021

mwaggett Aug 30, 2021

reidmv Sep 13, 2021

mwaggett Aug 30, 2021

reidmv Sep 13, 2021 •

edited

Loading

mwaggett Aug 30, 2021

reidmv Sep 13, 2021

mwaggett Aug 30, 2021

mwaggett Aug 30, 2021

reidmv Sep 13, 2021

mwaggett Aug 30, 2021

reidmv Sep 13, 2021 •

edited

Loading

mwaggett Aug 30, 2021

reidmv Sep 13, 2021

mwaggett Aug 30, 2021

reidmv Sep 13, 2021

reidmv Sep 13, 2021

reidmv Sep 14, 2021

reidmv Sep 17, 2021

mwaggett commented Aug 30, 2021

reidmv commented Sep 17, 2021

reidmv commented Oct 4, 2021

reidmv commented Nov 8, 2021

	def insync?(target_dir, purge: true)
	def in_sync?(target_dir, purge: true)

Tarball #1159

Tarball #1159

Conversation

reidmv commented May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reidmv Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reidmv Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwaggett commented Aug 30, 2021

reidmv commented Sep 17, 2021

reidmv commented Oct 4, 2021

reidmv commented Nov 8, 2021

reidmv commented May 6, 2021 •

edited

Loading

reidmv Sep 13, 2021 •

edited

Loading

reidmv Sep 13, 2021 •

edited

Loading