Skip to content

Conversation

@jpartlow
Copy link
Contributor

@jpartlow jpartlow commented May 21, 2025

Pull Request (PR) description

By default, curl captures as the -o file whatever the server response is
for <500 responses (excepting -L redirects), which means that a 404 file
not found error doesn't bubble up from the task until we later try to do
something with the file, like asking rpm or dpkg to install the server's
html 404 response text...

Adding --fail-with-body still captures the response but returns non-zero
and let's us halt at the download() where we want.

Additional work in the pr:

  • Adds a little BashRspec library for simple unit testing of functions
    in the files/common.sh library
  • Adds a set of unit tests under spec/unit/bash/common_sh_spec.rb
  • Adds some helper context for the tests to spec/lib/contexts.rb
  • Tighter constraint on which package managers to attempt based on the
    os_family variable with some specs.

This Pull Request (PR) fixes the following issues

Fixes #13

jpartlow added 4 commits May 20, 2025 14:48
Since modulesync is controlling the Gemfile absolutely, there's no hook
available to pull in the openvox gem via the puppet-stub workaround.
By default, curl captures as the -o file whatever the server response is
for <500 responses (excepting -L redirects), which means that a 404 file
not found error doesn't bubble up from the task until we later try to do
something with the file, like asking rpm or dpkg to install the server's
html 404 response text...

Adding --fail-with-body still captures the response but returns non-zero
and let's us halt at the download() where we want.
* Adds a little BashRspec library for simple unit testing of functions
  in the files/common.sh library
* Adds a set of unit tests under spec/unit/bash/common_sh_spec.rb
* Adds some helper context for the tests to spec/lib/contexts.rb
Tighter constraint on which package managers to attempt based on the
os_family variable with some specs.

# Mirrors the output from puppetlabs-facts.
module OBRspecFacts
UBUNTU_2404 = {
Copy link
Member

Choose a reason for hiding this comment

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

This looks a bit like you're rebuilding rspec-puppet-facts / facterdb?

https://github.com/voxpupuli/rspec-puppet-facts / https://github.com/voxpupuli/facterdb

Those tools provide you already factsets that you can iterate on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly, the facterdb facts provide all the facts facter would, I think. The facts in play here are a tiny subset of the os facts that the puppetlabs-facts facts task scrapes up on a system without an agent. Although you could certainly write an adapter to pull out os from the facterdb facts, and massage if needed, which would probably be a good idea if we find we need a more exhaustive set here. I don't think these specs need to test against all platforms, but might be good to expand it and refactor when we come back for sles, macos?

I can add a TODO reminder to that effect if you like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, extracting from facterdb for this probably is super simple; let me see what that looks like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I did fiddle with this a bit, but there are enough differences in the facts that I don't think it's worth it right now. For example:

Ubuntu 24.04
from facts/tasks/bash.sh

    os: {
      name: 'Ubuntu',
      distro: {
        codename: 'noble',
      },
      release: {
        full: '24.04',
        major: '24',
        minor: '04',
      },
      family: 'Debian',
    },
  }

from facterdb (grabbing the first out of the array of minors):

{:os=>                                                                                                                                              
   {"architecture"=>"amd64",                                                                                                                         
    "distro"=>{"codename"=>"noble", "description"=>"Ubuntu 24.04.2 LTS", "id"=>"Ubuntu", "release"=>{"full"=>"24.04", "major"=>"24.04"}},            
    "family"=>"Debian",                                                                                                                              
    "hardware"=>"x86_64",                                                                                                                            
    "name"=>"Ubuntu",                                                                                                                                
    "release"=>{"full"=>"24.04", "major"=>"24.04"},                                                                                                  
    "selinux"=>{"enabled"=>false}}}                                                                                                                  

I don't think there's any gain in massaging these backwards to the more limited facts bash.sh version right now.

For the couple of package manager operations in common.sh, apt had
preference before apt-get when searching for a system tool. But, from
what I've read, apt is not a replacement for apt-get, and the
expectation is not that newer Debian systems will eventually have apt
instead of apt-get. The apt tool is intended as a more user friendly
interface, while apt-get remains a stabler UI and more complete toolset
better suited for scripting. For this particular script, we aren't doing
any parsing, so it's academic. But given that we're not relying on a
behavior difference between the two, I've switched the order to favor
apt-get to adhere to the spirit of their differences.

I'm actually not aware of an installation where you would have apt-get
without apt or vice-versa, so a fallback may be unnecessary as well, but
there is undoubtadely some edge case somewhere...
@jpartlow jpartlow merged commit acd3c87 into main May 28, 2025
37 checks passed
@jpartlow jpartlow deleted the gh-13-curl-404 branch May 28, 2025 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix curl error handling

3 participants