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

(GH-187) Fixes abs failing provision if inventory file exists #190

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

jpartlow
Copy link
Contributor

I introduced this in cd72bb6.

When the newly structured abs task calls lib/task_helper's
get_inventory_hash() method and finds a pre-existing inventory_full_path
file. It attempts to call
PuppetLitmus::InventoryManipulation.inventory_hash_from_inventory_file()
which fails, because that method is not a class method.

The puzzling thing about this is why it ever worked, because I was wrong
that #187 was pre-existing.

The issue here is that the abs (the previous version), docker,
docker_exp, and vagrant tasks all use 'include PuppetLitmus::InventoryManipulation'
or 'include PuppetLitmus' (which includes InventoryManipulation) in the
scope of global functions declared implicitly on the main Object. This
causes the included module to be included in the base Object class,
which effectively makes its methods available everywhere. So a call to
PuppetLitmus::InventoryManipulation.inventory_hash_from_inventory_file()
actually works, as would String.inventory_hash_from_inventory_file or
anything else...

When I 'fixed' the abs task to include PuppetLitmus::InventoryManipulation
within the new ABSProvision class, I broke that global behavior
but only for abs.

The fix is straightforward now that I know what's going on. Removing the
module prefix from the method call works for both the new abs task and
the other tasks that still rely on the global behavior. In the abs task
case, InventoryManipulation is included in the class, so the methods are
available. For the other tasks, InventoryManipulation is still included
by main, and the methods are available everywhere anyway.

I introduced this in cd72bb6.

When the newly structured abs task calls lib/task_helper's
get_inventory_hash() method and finds a pre-existing inventory_full_path
file. It attempts to call
PuppetLitmus::InventoryManipulation.inventory_hash_from_inventory_file()
which fails, because that method is not a class method.

The puzzling thing about this is why it ever worked, because I was wrong
that puppetlabs#187 was pre-existing.

The issue here is that the abs (the previous version), docker,
docker_exp, and vagrant tasks all use 'include PuppetLitmus::InventoryManipulation'
or 'include PuppetLitmus' (which includes InventoryManipulation) in the
scope of global functions declared implicitly on the main Object. This
causes the included module to be included in the base Object class,
which effectively makes its methods available everywhere. So a call to
PuppetLitmus::InventoryManipulation.inventory_hash_from_inventory_file()
actually works, as would String.inventory_hash_from_inventory_file or
anything else...

When I 'fixed' the abs task to include PuppetLitmus::InventoryManipulation
within the new ABSProvision class, I broke that global behavior
but only for abs.

The fix is straightforward now that I know what's going on. Removing the
module prefix from the method call works for both the new abs task and
the other tasks that still rely on the global behavior. In the abs task
case, InventoryManipulation is included in the class, so the methods are
available. For the other tasks, InventoryManipulation is still included
by main, and the methods are available everywhere anyway.
@jpartlow jpartlow requested a review from a team as a code owner February 12, 2022 01:38
@jpartlow
Copy link
Contributor Author

I can't write specs for docker or vagrant tasks without refactoring them as well, but here's a manual run of provision::docker testing the patch:

jpartlow@jpartlow-dev:~/work/src/provision$ bundle exec bolt --modulepath spec/fixtures/modules
/ task run provision::docker --targets localhost  action=provision platform=ubuntu:14.04 invent
ory=/tmp
Bolt might be installed as a gem. To use Bolt reliably and with all of its
dependencies, uninstall the 'bolt' gem and install Bolt as a package:
https://puppet.com/docs/bolt/latest/bolt_installing.html

If you meant to install Bolt as a gem and want to disable this warning,
set the BOLT_GEM environment variable. [ID: gem_install]
Started on localhost...
Finished on localhost:
  {
    "status": "ok",
    "node_name": "localhost:52520",
    "node": {
      "uri": "localhost:52520",
      "config": {
        "transport": "ssh",
        "ssh": {
          "user": "root",
          "password": "root",
          "port": 52520,
          "host-key-check": false
        }
      },
      "facts": {
        "provisioner": "docker",
        "container_name": "ubuntu_14_04-52520",
        "platform": "ubuntu:14.04",
        "os-release": {
          "NAME": "Ubuntu",
          "VERSION": "14.04.6 LTS, Trusty Tahr",
          "ID": "ubuntu",
          "ID_LIKE": "debian",
          "PRETTY_NAME": "Ubuntu 14.04.6 LTS",
          "VERSION_ID": "14.04",
          "HOME_URL": "http://www.ubuntu.com/",
          "SUPPORT_URL": "http://help.ubuntu.com/",
          "BUG_REPORT_URL": "http://bugs.launchpad.net/ubuntu/"
        }
      }
    }
  }
Successful on 1 target: localhost
Ran on 1 target in 30.09 sec

If there's a Jenkins pipeline for provision I should check up on, let me know.

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@45b1290). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #190   +/-   ##
=======================================
  Coverage        ?   77.30%           
=======================================
  Files           ?        2           
  Lines           ?      141           
  Branches        ?        0           
=======================================
  Hits            ?      109           
  Misses          ?       32           
  Partials        ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45b1290...ee60a2f. Read the comment docs.

Copy link
Contributor

@chelnak chelnak left a comment

Choose a reason for hiding this comment

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

Approved. Thanks for sorting this out & thanks for posting the test evidence for docker & vagrant. I'll double check if there is a Jenkins pipeline out there.

@jpartlow jpartlow merged commit 1d59827 into puppetlabs:main Feb 14, 2022
@jpartlow jpartlow deleted the maint-fix-187 branch February 14, 2022 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants