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

replacing blacklist by blocklist and whitelist by allowlist #744

Closed
wants to merge 1 commit into from

Conversation

aponb
Copy link
Contributor

@aponb aponb commented May 20, 2021

Signed-off-by: Andreas Pre aponb@gmx.at

Description

opensearch should be open enough to avoid the words "blacklist" and "whitelist"

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Andreas Pre aponb@gmx.at

Signed-off-by: Andreas Pre <aponb@gmx.at>
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success d8f325c

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success d8f325c

@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed d8f325c

@aponb
Copy link
Contributor Author

aponb commented May 20, 2021

This pull request supersedes #729.
I learned from dblock that there is the wish to avoid racial stereotyping, but it is also necessary to preserve backwards compatibility. This pull request removes the words blacklist and whitelist from the complete source code. There are just a few settings, which contain these words. If somebody is using these settings, it shouldn't be a problem to change it. You can not get rid of these stereotypes for free, but I am sure people would understand and would change their settings for this.

@CEHENKLE
Copy link
Member

@aponb Thanks very much for doing this! I really appreciate it.

As @dblock said, we're trying really hard not to introduce any breaking changes in 1.0 (which is why we're moving changes like #472 to 2.0). But these are absolutely the changes I want to make, because this is the kind of software (inclusive) that we want to build.

So thanks again. :)

@dblock
Copy link
Member

dblock commented May 21, 2021

@aponb To get this merged you would need to add fallback for the settings to the old names in code. For example processors_whitelist.txt being loaded becomes processors_allowlist by default, but can fallback to processors_whitelist.txt if present with a deprecation warning. We can then ensure we delete the fallback in a major version upgrade in the future.

I would also suggest maybe splitting the work. We could first merge a change that changes identifiers but not actual things like file names to preserve backwards compatibility. That would significantly reduce the amount of places we see these keywords in the codebase, and the work we'd have to do will be lesser later.

private static final Allowlist ALLOWLIST = AllowlistLoader.loadFromResourceFiles(ProcessorsAllowlistExtension.class, "processors_whitelist.txt");

When we have only actual backwards compatibility questions, we can take them 1-by-1. For example, I am not sure processors_whitelist.txt is something that people actually set themselves, I think we may be shipping this as part of the distribution. In which case if it's internal we can just rename it. But I'd want to be sure in order not to break anyone's upgrade path.

Does this help?

@aponb
Copy link
Contributor Author

aponb commented May 27, 2021

Sorry for the late response. That is definitely a way to go. In a first shot I am going to remove all old names from the code base. Then we can look out for the rest and do some fallback scenarios.

@nknize nknize added >breaking Identifies a breaking change. enhancement Enhancement or improvement to existing feature or request v2.0.0 Version 2.0.0 labels Jun 25, 2021
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed d8f325c

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Wrapper Validation success d8f325c

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Precommit success d8f325c

@dblock
Copy link
Member

dblock commented Jul 6, 2021

Sorry for the late response. That is definitely a way to go. In a first shot I am going to remove all old names from the code base. Then we can look out for the rest and do some fallback scenarios.

@aponb I hope you find time to pick this up!

@aponb
Copy link
Contributor Author

aponb commented Jul 7, 2021

Yes, I am going to do it in the next days!

@opensearch-ci-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@CEHENKLE
Copy link
Member

@aponb Hey! Just checking in to see if you've had a chance to take a look at this. Thanks!

@aponb
Copy link
Contributor Author

aponb commented Oct 30, 2021

@aponb Hey! Just checking in to see if you've had a chance to take a look at this. Thanks!

sorry for being so late. Going to pick up that topic in the next weeks!

@tlfeng
Copy link
Collaborator

tlfeng commented Nov 10, 2021

Hi @aponb, we have got a plan to replace the non-inclusive term "blacklist / whitelist" (#1483), could you please take a look at the plan and modify your PR?
To sum up,

  1. Could you use denylist to replace blacklist? We think it's more accurate as the opposite to allowlist.
  2. We would like to resolve the issue Replace "blacklist/whitelist" terminology in code comments, and internal variable, method and class names #1484 as the first step, so please split your PR to have one that only replacing the "blacklist / whitelist" where the backwards compatibility won't be impacted, and those without touching APIs or settings.

Look forward to hearing from you soon. Thank you! 👍

@aponb
Copy link
Contributor Author

aponb commented Nov 11, 2021

ok thanks, I will stick to that plan. Probably I will have new Pull Requests, as this one is harder to change. Keep you informed!

@tlfeng
Copy link
Collaborator

tlfeng commented Nov 12, 2021

ok thanks, I will stick to that plan. Probably I will have new Pull Requests, as this one is harder to change. Keep you informed!

@aponb Thank you for your help and your understanding! 😁 👍 .

@tlfeng
Copy link
Collaborator

tlfeng commented Jan 10, 2022

Hi @aponb, could you please share any plan or progress on this issue? Thank you!

@aponb
Copy link
Contributor Author

aponb commented Jan 12, 2022

Hi @aponb, could you please share any plan or progress on this issue? Thank you!

I could make a new pull request with changes of replacing the exclusionary words that won't impact the compatibility. Changes just in code comments, internal variables, methods and class names. In doubt I kept the old wording. But a lot of the words are gone that way. I you want I could post the new pull request.

@tlfeng
Copy link
Collaborator

tlfeng commented Jan 24, 2022

I could make a new pull request with changes of replacing the exclusionary words that won't impact the compatibility. Changes just in code comments, internal variables, methods and class names. In doubt I kept the old wording. But a lot of the words are gone that way. I you want I could post the new pull request.

Hi @aponb, that sounds good! Look forward to see your new pull request soon. 😄 Let's start from replacing the non-inclusive term "blacklist/whitelist" in code comments or internal variable/method/class names, and then if you have time, you could go on following our plan.
We'd like to introduce the replacement for the non-inclusive terms in OpenSearch 1.3.0 release, which will be released in this March (https://github.com/orgs/opensearch-project/projects/1). We can provide any help to you regarding this code change.
Thank you!

@aponb
Copy link
Contributor Author

aponb commented Jan 24, 2022

Hi @aponb, that sounds good! Look forward to see your new pull request soon.

it's Pull request #1970

@tlfeng
Copy link
Collaborator

tlfeng commented Jan 24, 2022

it's Pull request #1970

@aponb, Thanks a lot for your time! That's so quick to see the new Pull request. 👍👍

@dblock
Copy link
Member

dblock commented Mar 3, 2022

@tlfeng What should we do with this PR?

@tlfeng
Copy link
Collaborator

tlfeng commented Mar 3, 2022

@tlfeng What should we do with this PR?

Hi @dblock , thanks for the reminder. The PR can be closed.
The changes in this PR will be covered in 3 separate issues (mentioned in #1483):
#1484
#1547
#1683

@tlfeng tlfeng closed this Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking Identifies a breaking change. enhancement Enhancement or improvement to existing feature or request v2.0.0 Version 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants