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

add "packages" resource #1458

Merged
merged 1 commit into from
Feb 7, 2017
Merged

add "packages" resource #1458

merged 1 commit into from
Feb 7, 2017

Conversation

jtimberman
Copy link

@jtimberman jtimberman commented Feb 3, 2017

This pull request adds a packages resource so that we can check for pattern matches against all the packages on a system. This initially implements only dpkg support for debian-based platforms so we can cover this use case:

describe packages(/^xserver-xorg.*/) do
  its("list") { should be_empty }
end

This uses FilterTable so we can supply additional queries, too.

describe packages(/vi.+/).where { status != 'installed' } do
  its('statuses') { should be_empty }
end

Users can specify the name as a string or a regular expression. If it is a string, we will escape it and convert it to a regular expression to use in matching against the full returned list of packages. If it is a regular expression, we take that as is and use it to filter the results.

While some package management systems such as dpkg can take a shell glob argument to filter their results, we eschew this and require a regular expression to match multiple package names because we will need this to work across other platforms in the future. This means that the following:

packages("vim")

Will return all the "vim" packages on the system. The packages resource will take "vim", turn it into /vim/, and greedily match anything with "vim" in the name. To match only a single package named vim, it needs to be an anchored regular expression.

packages(/^vim$/)

@alexpop
Copy link
Contributor

alexpop commented Feb 6, 2017

I added a few commits to use the more standard entries, add a few more tests, fix lint.

list as an attribute only works on the main resource(i.e. packages("xserver-xorg*").list) where entries works on the main resource + on where filter(i.e. packages("xserver-xorg*").where { status == 'installed' }.entries)

# a regular expression. let's make sure that the pattern gets
# converted to a regular expression
def pattern_regexp(p)
shmap = { '*'=>'.*', '?' => '.', '[' => '[', ']' => ']' }
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the deal with the "[" and "]" replaces? They don't seem to do much:

[9] pry(main)> "vim][".gsub(/(.)/) { |w| shmap[w] || w }
=> "vim]["

return [] if all.nil?
all.map do |m|
a = m.split
a[0] = 'installed' if a[0] =~ /^ii/
Copy link
Contributor

Choose a reason for hiding this comment

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

So only installed status is expanded, what about these combinations?

 Desired action:
    u = Unknown
    i = Install
    h = Hold
    r = Remove
    p = Purge

  Package status:
    n = Not-installed
    c = Config-files
    H = Half-installed
    U = Unpacked
    F = Half-configured
    W = Triggers-awaiting
    t = Triggers-pending
    i = Installed

I've added in the output two packages what have the two status characters rc

Copy link
Contributor

@arlimus arlimus Feb 6, 2017

Choose a reason for hiding this comment

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

Ignore the desired action for now. Let's focus on the actual state of the package. We will keep them separate in the future. I.e.:

match(/^([uihrp])([ncHUFWti])\s/)
=> res[1] we will ignore for now, it is part of a future field for desired state
=> res[2] is the status, we support installed right now

We will establish installed as a shared status across all OS. The others are ok to be os-specific for now. imho implement as lowercase with spaces, e.g. not installed. Do not export the other status types for now, just stick to installed + open the others in a RFC. Sync with the package resource.

Copy link
Contributor

@arlimus arlimus left a comment

Choose a reason for hiding this comment

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

Awesome work thank you so much!! 😄 Just 2 tiny comments

return [] if all.nil?
all.map do |m|
a = m.split
a[0] = 'installed' if a[0] =~ /^ii/
Copy link
Contributor

@arlimus arlimus Feb 6, 2017

Choose a reason for hiding this comment

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

Ignore the desired action for now. Let's focus on the actual state of the package. We will keep them separate in the future. I.e.:

match(/^([uihrp])([ncHUFWti])\s/)
=> res[1] we will ignore for now, it is part of a future field for desired state
=> res[2] is the status, we support installed right now

We will establish installed as a shared status across all OS. The others are ok to be os-specific for now. imho implement as lowercase with spaces, e.g. not installed. Do not export the other status types for now, just stick to installed + open the others in a RFC. Sync with the package resource.

ii xz-utils 5.1.1alpha+20120614-2ubuntu2
ii zerofree 1.0.3-1
ii zlib1g 1:1.2.8.dfsg-2ubuntu4
ii zlib1g-dev 1:1.2.8.dfsg-2ubuntu4
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's shorten this just to the necessary lines

@arlimus
Copy link
Contributor

arlimus commented Feb 6, 2017

Just one more thing: I think we should keep the string as a non-regex only check. i.e. no glob expression. Keep regex'y behavior to real regex input for the packages initialization. Kudos again!

@jtimberman
Copy link
Author

Don't merge yet - I'm going to rebase and rewrite the original commit message to match reality.

@jtimberman jtimberman self-assigned this Feb 6, 2017
@jtimberman
Copy link
Author

This is ready for review/merge.

This pull request adds a packages resource so that we can check for pattern matches against all the packages on a system. This initially implements only dpkg support for debian-based platforms so we can cover this use case:

```ruby
describe packages(/^xserver-xorg.*/) do
  its("list") { should be_empty }
end
```

This uses FilterTable so we can supply additional queries, too.

```ruby
describe packages(/vi.+/).where { status != 'installed' } do
  its('statuses') { should be_empty }
end
```

Users can specify the name as a string or a regular expression. If it is a string, we will escape it and convert it to a regular expression to use in matching against the full returned list of packages. If it is a regular expression, we take that as is and use it to filter the results.

While some package management systems such as `dpkg` can take a shell glob argument to filter their results, we eschew this and require a regular expression to match multiple package names because we will need this to work across other platforms in the future. This means that the following:

```ruby
packages("vim")
```

Will return *all* the "vim" packages on the system. The `packages` resource will take `"vim"`, turn it into `/vim/`, and greedily match anything with "vim" in the name. To match only a single package named `vim`, it needs to be an anchored regular expression.

```ruby
packages(/^vim$/)
```

Signed-off-by: Joshua Timberman <joshua@chef.io>

Use entries instead of list

Added a few more tests and non installed package in output
Signed-off-by: Alex Pop <apop@chef.io>

fix lint

Signed-off-by: Alex Pop <apop@chef.io>

Signed-off-by: Joshua Timberman <joshua@chef.io>
@alexpop alexpop force-pushed the jtimberman/packages-resource branch from 707dc18 to d7fad68 Compare February 7, 2017 10:29
@alexpop
Copy link
Contributor

alexpop commented Feb 7, 2017

👍 for the current version. Thanks for the great work Joshua!

@arlimus
Copy link
Contributor

arlimus commented Feb 7, 2017

Awesome work and huge kudos for adding it Joshua!!
👍

@arlimus arlimus merged commit e41ec1f into master Feb 7, 2017
@arlimus arlimus deleted the jtimberman/packages-resource branch February 7, 2017 12:59
@arlimus arlimus added Type: Enhancement Improves an existing feature and removed in progress labels Feb 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Improves an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants