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

(CODEMGMT-1323) Prefer purge_allowlist #1144

Merged
merged 1 commit into from
Apr 12, 2021

Conversation

tvpartytonight
Copy link
Contributor

Please add all notable changes to the "Unreleased" section of the CHANGELOG in the format:

- Summary of changes. [Issue or PR #](link to issue or PR)

@tvpartytonight
Copy link
Contributor Author

This is the necessary change for removing writing 'purge_whitelist' to disk in code-manager. If this looks good, there are some doc changes to put in and also I'd like to change one of the acceptance tests to use purge_allowlist instead of purge_whitelist.

There's other work here to do to remove 'whitelist' in general, but this should enable better completion for our goals for Puppet.

whitelist_has_content = !whitelist.empty?
allowlist_has_content = !allowlist.empty?
case
when whitelist_has_content == false && allowlist_has_content == false
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) I believe it's conventional for the whens to line up with case.

Comment on lines 54 to 55
raise R10K::Error "Values found for both purge_whitelist and purge_allowlist. Setting " <<
"purge_whitelist is deprecated, please move all values to purge-allowlist."
Copy link
Contributor

Choose a reason for hiding this comment

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

same question as in the code-manager PR: are we sure it's preferable to disallow both keys, from an upgrading perspective?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've rejiggered the CM PR so this isn't an issue; it seems to me that if both are defined, than the upgrade hasn't been implemented correctly. I'm open to being more lax, but it seems more correct to error when two values are presented trying to configure the same thing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm with Tony, I don't think there's a reason to allow both simultaneously. We need to still support both keys, but should only allow you to have one or the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay

Comment on lines 126 to 128
:desc => "A list of filename patterns to be excluded from any purge operations. Patterns are matched relative to the root of each deployed environment, if you want a pattern to match recursively you need to use the '**' glob in your pattern. Basic shell style globs are supported.",
:default => [],
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) this indentation doesn't match what's done in the rest of this file

@mwaggett
Copy link
Contributor

mwaggett commented Apr 9, 2021

Your commit message says allowlevel instead of allowlist 😅 it could also use a little more detail, I think.

This will also ultimately need a changelog update as well.

This change deprecates the setting `purge_whitelist` and prefers
the setting `purge_allowlist`. The setting `purge_whitelist` will
still work until a future change removes it entirely.
@tvpartytonight tvpartytonight marked this pull request as ready for review April 12, 2021 21:31
@tvpartytonight tvpartytonight requested a review from a team April 12, 2021 21:31
@tvpartytonight tvpartytonight changed the title (CODEMGMT-1323) Prefer purge_allowlevel (CODEMGMT-1323) Prefer purge_allowlist Apr 12, 2021
@Magisus Magisus merged commit e89cd7d into puppetlabs:main Apr 12, 2021
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.

3 participants