Tarball #1244

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

Chunk PR in support of (original monster PR) #1159

This pull request DEPENDS ON #1242, #1243. Do not merge until after those PRs are merged.

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.

Fixes #567
Closes #1159

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

Magisus

I think I want to play with this a bit.

lib/r10k/module/tarball.rb

spec/unit/module/tarball_spec.rb

lib/r10k/environment/tarball.rb

Magisus · 2021-11-12T19:25:02Z

Would you mind adding something to the docs with examples on how to use this?

Magisus · 2021-11-15T22:13:09Z

lib/r10k/environment/tarball.rb

+  # @return [Array<String>]
+  def desired_contents
+    desired = []
+    desired += @tarball.paths.map { |entry| File.join(@full_path, entry) }


Discovered an interesting edge case here: if you build the tarball a bit wrong, even though it unpacks correctly, it might purge incorrectly. In my case, I created the tarball with .. So the paths inside it look like this:

"/Users/aileen/code/r10k/yaml-envs/tarball/.", "/Users/aileen/code/r10k/yaml-envs/tarball/./site-modules", "/Users/aileen/code/r10k/yaml-envs/tarball/./hiera.yaml", ...

But r10k's current_contents list looks like this:

"/Users/aileen/code/r10k/yaml-envs/tarball/site-modules", "/Users/aileen/code/r10k/yaml-envs/tarball/hiera.yaml", ...

So it purges everything except the modules directory (which is ignored by the environment purge level).

I realize this was my mistake, and I should get better at tarballs 😛 But it seems like something that could trip people up and cause a very confusing bug. Maybe we can add some normalization in Tarball.paths? Adding Pathname.new(path).cleanpath seemed to do the trick for me.

Ha! Excellent catch. Although I do have unbridled confidence that users of software would NEVER imperfectly construct a tarball 😉, this does seem like something we could fix.

Huh. I just snap-pushed what I thought would be the fix in 5a1711e, and then realized I was looking at a completely different part of the code. But, it might actually be a valid fix anyway? Don't have a test for it yet.

That's the spot I edited when fixing this locally 🤷‍♀️ Seemed to work for my bad tarball.

Were you working on a test for this? I think besides that, I'm happy to merge this.

Yeah, lemme add some thing for that. I might just re-construct the fixture tarball to include the ./ name components, I think that'd make it easy to validate. Will push something shortly.

Yeah that's what I was thinking 🙂

Magisus · 2021-11-16T17:27:57Z

So, are the tarball modules usable from the Puppetfile too? Or just with yaml envs? If they are usable from a Puppetfile, it might be nice to note that here.

reidmv · 2021-11-16T17:44:04Z

So, are the tarball modules usable from the Puppetfile too? Or just with yaml envs? If they are usable from a Puppetfile, it might be nice to note that here.

Totally, and yeah, they are usable. I believe bc24f6b contains some reference to that. Do you think it should say more or say it differently?

Magisus · 2021-11-16T17:49:03Z

Oh duh, no, I missed which file that got added to, my bad.

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

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

Co-authored-by: Maggie Dreyer <maggie@puppet.com>

Users might construct a tarball with entries like this: ./ ./foo.txt ./foo/bar/ r10k's purging facility needs these to be normalized to work correctly. This commit makes the R10K::Tarball#paths method normalize the values to e.g. foo.txt foo/bar Co-authored-by: Maggie Dreyer <maggie@puppet.com>

reidmv · 2021-11-16T18:27:14Z

Two logical changes:

Changed the fixture'd tarball to include ./ path prefixes, such as a user might incidentally include in a constructed tarball. Did this in a rebase so as not to leave an extra binary blob in the Git history.
Made a new commit that adjusts R10K::Tarball#paths to return clean paths only, with a test

I didn't go so far as adding a purge test specifically. Seems like the most thorough fix would actually be to R10K::Util::Purgable itself, if we really wanted to do that. It seems reasonable enough for Tarball to just make sure paths returned are clean paths.

Magisus

Retested this, seems to work well.

justinstoller · 2021-11-16T19:20:31Z

lib/r10k/module/tarball.rb

+  def properties
+    {
+      :expected => @checksum,
+      :actual   => status,


This would be something like :insync, :absent, :mismatch, right? I think consumers of this (for existing modules) determine the status of insync/mismatch/absent from whether the actual equals the expected, or if it differs, and if it differs if the actual was nil, respectively. I think we want to continue that behavior.

Better? e783dcc

I'm not actually sure how/where/why #properties is used, or what expectations exist for this method. The YARDoc definition in R10K::Module::Base isn't super helpful on that front.

They are collected to write the r10k-deploy.json, which is used by Impact Analysis.

When a module is insync, the module's `actual` and `expected` properties should match. When the module is not in sync, they should differ.

reidmv requested a review from a team November 8, 2021 20:25

reidmv mentioned this pull request Nov 8, 2021

Tarball #1159

Closed

reidmv force-pushed the tarball-refactor branch from c9159b3 to 1ee9767 Compare November 10, 2021 00:18

Update r10k.gemspec minitar dependency

c671411

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

reidmv force-pushed the tarball-refactor branch 2 times, most recently from 1d73966 to bd759f7 Compare November 10, 2021 23:55

mwaggett added awaiting codeowner action feature labels Nov 11, 2021

Magisus reviewed Nov 12, 2021

View reviewed changes

Magisus reviewed Nov 15, 2021

View reviewed changes

reidmv force-pushed the tarball-refactor branch from 5a1711e to a059c4c Compare November 16, 2021 18:18

reidmv and others added 7 commits November 16, 2021 10:23

Implement R10K::Tarball

197bee4

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

Update initializers

618385c

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

Incorporate feedback from review

84a2b6a

Co-authored-by: Maggie Dreyer <maggie@puppet.com>

Add tarball module and environment type docs

c72de49

reidmv force-pushed the tarball-refactor branch from a059c4c to 7f1683d Compare November 16, 2021 18:23

Magisus approved these changes Nov 16, 2021

View reviewed changes

justinstoller reviewed Nov 16, 2021

View reviewed changes

Normalize tarball module properties return

e783dcc

When a module is insync, the module's `actual` and `expected` properties should match. When the module is not in sync, they should differ.

justinstoller approved these changes Nov 16, 2021

View reviewed changes

Magisus approved these changes Nov 16, 2021

View reviewed changes

Magisus merged commit 8cb3643 into puppetlabs:main Nov 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tarball #1244

Tarball #1244

reidmv commented Nov 8, 2021

Magisus left a comment

Magisus commented Nov 12, 2021

Magisus Nov 15, 2021

reidmv Nov 15, 2021

reidmv Nov 15, 2021

Magisus Nov 15, 2021

Magisus Nov 16, 2021

reidmv Nov 16, 2021

Magisus Nov 16, 2021

Magisus commented Nov 16, 2021 •

edited

Loading

reidmv commented Nov 16, 2021

Magisus commented Nov 16, 2021

reidmv commented Nov 16, 2021

Magisus left a comment

justinstoller Nov 16, 2021

reidmv Nov 16, 2021

justinstoller Nov 16, 2021

Tarball #1244

Tarball #1244

Conversation

reidmv commented Nov 8, 2021

Magisus left a comment

Choose a reason for hiding this comment

Magisus commented Nov 12, 2021

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

Magisus commented Nov 16, 2021 • edited Loading

reidmv commented Nov 16, 2021

Magisus commented Nov 16, 2021

reidmv commented Nov 16, 2021

Magisus left a 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

Magisus commented Nov 16, 2021 •

edited

Loading