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

*: s/whitelist/allowlist/, s/blacklist/blocklist/ #49946

Merged
merged 1 commit into from
Jun 9, 2020

Conversation

jordanlewis
Copy link
Member

@jordanlewis jordanlewis commented Jun 7, 2020

Allow and block aren't hurtful terms, and they also happen to be clearer
than white and black - they connote meaning instantly.

Even though blacklist and whitelist don't have racist intent, they nevertheless
propagate an unconscious "white = allow", "black = deny" meaning. There's no
need to keep these terms - they're just terms. This is just one tiny thing we can
do to improve our industry's inclusivity.

c.f.:

https://twitter.com/bradfitz/status/1269449722063773696?s=20
rails/rails#33677
https://gitlab.com/gitlab-org/gitlab/-/issues/7554
lagom/lagom#2532
https://bugs.chromium.org/p/chromium/issues/detail?id=981129

Release note: None

@jordanlewis jordanlewis requested review from a team as code owners June 7, 2020 03:23
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
Copy link
Contributor

knz commented Jun 7, 2020

Please also add a linter for the future

@jordanlewis
Copy link
Member Author

Adding a reviewer for appdev and sql, since looks like we have approval from kv.

@jordanlewis jordanlewis requested review from rafiss and solongordon June 8, 2020 15:43
Copy link
Contributor

@solongordon solongordon left a comment

Choose a reason for hiding this comment

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

There are a few instances of "black list" and "white list" as well.

Reviewed 12 of 55 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, and @solongordon)


pkg/cmd/roachtest/blocklist_test.go, line 31 at r1 (raw file):

func TestBlocklists(t *testing.T) {
	if _, ok := os.LookupEnv(runBlocklistEnv); !ok {
		t.Skipf("Blackist test is only run if %s is set", runBlocklistEnv)

Missed due to misspelling

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, and @solongordon)


pkg/cmd/roachtest/blocklist_test.go, line 27 at r2 (raw file):

const githubAPITokenEnv = "GITHUB_API_TOKEN"
const runBlocklistEnv = "RUN_BLOCKLIST_TEST"

TBH, "blocklist" is not a very clear term to me, at least for these ORM nightly tests. (I can't speak for every other test in the repo). in fact, i think the only meaning it has to me is because it sounds very similar to "blacklist."

would a term like "expectedFailList" be ok for the ORM tests -- that's exactly what is being tracked in the lists.

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

thanks for thinking of this change!

is it missing a "Release note: none" ?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @solongordon)

Copy link
Contributor

@solongordon solongordon left a comment

Choose a reason for hiding this comment

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

Reviewed 17 of 55 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, and @solongordon)


pkg/cmd/roachtest/blocklist_test.go, line 27 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

TBH, "blocklist" is not a very clear term to me, at least for these ORM nightly tests. (I can't speak for every other test in the repo). in fact, i think the only meaning it has to me is because it sounds very similar to "blacklist."

would a term like "expectedFailList" be ok for the ORM tests -- that's exactly what is being tracked in the lists.

I'd say this is the perfect opportunity to choose more descriptive names like "expected fail list" on a case-by-case basis. I think "blocklist" is a reasonable starting place though. I agree it's vague but I would say no vaguer than "blacklist."

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

that sounds fine. i can change the ORM related tests after this is merged.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, and @solongordon)

@jordanlewis
Copy link
Member Author

Good point, I didn't actually think about all of the cases here, just used search and replace. But you're right that blocklist doesn't really apply here. I also agree that you can do that in another PR - thanks for volunteering @rafiss :)

Raphael, I think a linter is an interesting idea, but we also don't have linters for other phrases we avoid really, so why this one? I'd like to merge without a linter and see what happens. I'll do my part to remind colleagues to avoid blacklist/whitelist when it comes up, and you should too. I think there's a "When in Rome" argument to be made here - when I joined CockroachDB, I named the main TeamCity server the "teamcity leader" because I wanted to fit in with the leader/follower terminology we use here, even though it doesn't ... really make a ton of sense to call the main TeamCity server the leader in the same way a Raft leader is a leader. So, maybe I'm an optimist, but I don't think we need a linter. Can always revisit if it matters!

I fixed the missing cases and added a release note, so going to merge. TFTRs all.

bors r+

@craig
Copy link
Contributor

craig bot commented Jun 9, 2020

Build succeeded

@craig craig bot merged commit 3c75694 into cockroachdb:master Jun 9, 2020
@jordanlewis jordanlewis deleted the allowlist branch June 11, 2020 02:54
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.

6 participants