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 new boolean property anchor_package_regex to resource apt_package resource #13873

Merged
merged 4 commits into from Sep 28, 2023

Conversation

neha-p6
Copy link
Collaborator

@neha-p6 neha-p6 commented Aug 10, 2023

Description

Add new boolean property anchor_package_regex to resource apt_package.
For apt-cache command the name of the package is searched as a regex due to which it tries to implicitly install all the packages whose name would contain the passed name string as a substring.
This causes issues for people which who not want this to happen. Hence we add this new property, which will be false by default for backward compatibility. Users can set it to true in their recipe if they want the package name to be exactly matched instead. (i.e anchored with ^...$)
Documentation pr chef/chef-web-docs#4156

Related Issue

#12944

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commits have been signed-off for the Developer Certificate of Origin.

@neha-p6 neha-p6 requested review from a team as code owners August 10, 2023 12:59
@neha-p6 neha-p6 changed the title Add new boolean property anchor_package_name_patterns to resource apt_package resource [WIP]Add new boolean property anchor_package_name_patterns to resource apt_package resource Aug 10, 2023
@github-actions github-actions bot added the documentation How do we use this project? label Aug 10, 2023
@neha-p6 neha-p6 changed the title [WIP]Add new boolean property anchor_package_name_patterns to resource apt_package resource Add new boolean property anchor_package_name_patterns to resource apt_package resource Aug 14, 2023
@neha-p6 neha-p6 removed the documentation How do we use this project? label Aug 14, 2023
lib/chef/provider/package/apt.rb Outdated Show resolved Hide resolved
lib/chef/provider/package/apt.rb Outdated Show resolved Hide resolved
lib/chef/resource/apt_package.rb Outdated Show resolved Hide resolved
@jaymzh jaymzh added the Status: Waiting on Contributor A pull request that has unresolved requested actions from the author. label Aug 22, 2023
lib/chef/resource/apt_package.rb Outdated Show resolved Hide resolved
lib/chef/resource/apt_package.rb Outdated Show resolved Hide resolved
lib/chef/resource/apt_package.rb Outdated Show resolved Hide resolved
lib/chef/resource/apt_package.rb Outdated Show resolved Hide resolved
@neha-p6 neha-p6 changed the title Add new boolean property anchor_package_name_patterns to resource apt_package resource Add new boolean property anchor_package_regex to resource apt_package resource Aug 25, 2023
@github-actions github-actions bot added the documentation How do we use this project? label Aug 25, 2023
…ge. For apt-cache command the name of the package is

searched as a regex due to which it tries to implicitly install all the packages whose name would contain the passed name
string as a substring. This causes issues for people which who not want this to happen. Hence we add this new property,
which will be false by default for backword compatibility. Users can set it to true in their recipe if they want the
package name to be exactly matched instead

* Add documentation changes for apt_package resource

Signed-off-by: Neha Pansare <neha.pansare@progress.com>
@sonarcloud
Copy link

sonarcloud bot commented Aug 29, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
8.3% 8.3% Duplication

@@ -75,6 +90,10 @@ class AptPackage < Chef::Resource::Package
description: "A Hash of response file variables in the form of {'VARIABLE' => 'VALUE'}.",
default: {}, desired_state: false

property :anchor_package_regex, [TrueClass, FalseClass],
introduced: "18.3",
description: "A Boolean flag that allows (`false`) or prevents (`true`) apt_package from matching the named package with packages by regular expression if it can't find a package with the exact same name.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
description: "A Boolean flag that allows (`false`) or prevents (`true`) apt_package from matching the named package with packages by regular expression if it can't find a package with the exact same name.",
description: "A Boolean flag that wraps a package name in regex anchors to help prevent `apt_package` from installing packages with similar names.",

Copy link
Contributor

Choose a reason for hiding this comment

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

How does this prevent this? What do true and false do?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because the regex anchors specify that the regex pattern must include the entire package name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The larger description above provides lots of other context. But anything treated like a regular expression means it'll match substrings, so "apache" which would match "apache-mycoolthing" or whatever. This is the short-description, suggestions welcome

Copy link
Collaborator

Choose a reason for hiding this comment

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

This PR was merged with this change as well, and the description is not accurate as it stands. It claims it's not a regex anymore, but it still is, it's just an anchored one. If someone passes in foo.*bar that'll still work.

Copy link
Collaborator

@jaymzh jaymzh left a comment

Choose a reason for hiding this comment

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

Code is all great now - but the documentation was misleading, requested some changes.

@IanMadd
Copy link
Contributor

IanMadd commented Aug 29, 2023

Code is all great now - but the documentation was misleading, requested some changes.

What's wrong with it?

@neha-p6
Copy link
Collaborator Author

neha-p6 commented Aug 30, 2023

Code is all great now - but the documentation was misleading, requested some changes.

What's wrong with it?

@IanMadd Their suggestions
#13873 (comment)
#13873 (comment)

@rahulgoel1
Copy link
Collaborator

rahulgoel1 commented Sep 25, 2023

@IanMadd Just wanted to follow up on this PR. Is there anything pending from documentation perspective on this PR? @johnmccrae

Comment on lines +59 to +60
If it can't find a package with the exact same name, it will treat the package name as regex string and match with any package that matches that regex.
This may lead Chef Infra Client to install one or more packages with names that match that regex.
Copy link
Contributor

@IanMadd IanMadd Sep 27, 2023

Choose a reason for hiding this comment

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

https://developers.google.com/style/word-list#regex

Suggested change
If it can't find a package with the exact same name, it will treat the package name as regex string and match with any package that matches that regex.
This may lead Chef Infra Client to install one or more packages with names that match that regex.
If it can't find a package with the exact same name, it will treat the package name as regular expression string and match with any package that matches that regular expression.
This may lead Chef Infra Client to install one or more packages with names that match that regular expression.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This pr was merged without this change. Addressing that in a separate pr #13991

Signed-off-by: John McCrae <john.mccrae@progress.com>
Signed-off-by: John McCrae <john.mccrae@progress.com>
@sonarcloud
Copy link

sonarcloud bot commented Sep 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
8.3% 8.3% Duplication

@johnmccrae
Copy link
Contributor

johnmccrae commented Sep 28, 2023

Had to add a last minute gem update and rebase to get the PR to pull in the correct versions of some things. The error in the unit test is known and will be forward-ported from chef-17. #13926

@johnmccrae johnmccrae merged commit 0fdbf2f into main Sep 28, 2023
25 of 28 checks passed
@johnmccrae johnmccrae deleted the neha-p6/apt_pkg_query branch September 28, 2023 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation How do we use this project? Status: Waiting on Contributor A pull request that has unresolved requested actions from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants