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

Use alternate names for blacklist/whitelist in prow #19264

Closed
2 tasks
spiffxp opened this issue Sep 17, 2020 · 30 comments
Closed
2 tasks

Use alternate names for blacklist/whitelist in prow #19264

spiffxp opened this issue Sep 17, 2020 · 30 comments
Assignees
Labels
area/prow Issues or PRs related to prow good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. sig/testing Categorizes an issue or PR as relevant to SIG Testing.
Milestone

Comments

@spiffxp
Copy link
Member

spiffxp commented Sep 17, 2020

What should be cleaned up or changed:
There are sundry files within prow that can probably use more specific terms, or the generic terms "allowlist/denylist"

$ ag -l whitelist\|blacklist prow | sort
prow/ANNOUNCEMENTS.md # ignore, deprecation announcements
prow/cmd/exporter/collector.go
prow/cmd/exporter/collector_test.go
prow/cmd/hook/main.go
prow/cmd/status-reconciler/main.go # offending flag has been deprecated
prow/cmd/status-reconciler/main_test.go
prow/config/config.go
prow/config/prow-config-documented.yaml
prow/hook/hook_test.go
prow/plugins/approve/approve_test.go
prow/plugins/blunderbuss/blunderbuss_test.go
prow/plugins/config.go
prow/plugins/lgtm/lgtm_test.go
prow/plugins/plugin-config-documented.yaml
prow/plugins/verify-owners/verify-owners.go
prow/plugins/verify-owners/verify-owners_test.go
prow/repoowners/repoowners.go
prow/repoowners/repoowners_test.go

EDIT: pulling up from #19264 (comment)

All instances of whitelist have been removed from prow (thanks @chases2!). blacklist appears in two config fields that would need to be deprecated:

  • OwnersDirBlackList, in the Prow config
  • LabelsBlacklist, in the Prow plugins config

Provide any links for context:

/area prow
/wg naming
/sig testing

@spiffxp spiffxp added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Sep 17, 2020
@k8s-ci-robot k8s-ci-robot added area/prow Issues or PRs related to prow wg/naming Categorizes an issue or PR as relevant to WG Naming. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Sep 17, 2020
@justaugustus
Copy link
Member

Needs a rec proposed for replacement.
cc: @kubernetes/wg-naming

@justaugustus
Copy link
Member

/help

@k8s-ci-robot
Copy link
Contributor

@justaugustus:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Nov 4, 2020
@justaugustus
Copy link
Member

For ADR:
/assign @celestehorgan

For advice on impl:
/assign @justaugustus

@justaugustus
Copy link
Member

ADR up for review: kubernetes/community#5341

@chases2
Copy link
Contributor

chases2 commented Jan 27, 2021

I went through the files in the prow dir and renamed the easier ones. I managed to remove all of the instances of "whitelist", but "blacklist" has two configurations that would need to be deprecated:

  • OwnersDirBlackList, in the Prow config
  • LabelsBlacklist, in the Prow plugins config

Also notable:

  • The file ANNOUNCEMENTS.md contains deprecation announcements, which shouldn't be changed.
  • status-reconciler has already had its flag deprecated, with a tombstone date of June 1st.
    • After June 1st, delete the flag

@spiffxp
Copy link
Member Author

spiffxp commented Mar 5, 2021

ref: #18801 (covered slack mergewarning plugin)

@chaodaiG
Copy link
Contributor

/assign

@chaodaiG
Copy link
Contributor

#21577 and #21580 are for deprecating the remaining two "blacklist" config fields

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 28, 2021
@spiffxp
Copy link
Member Author

spiffxp commented Jul 27, 2021

/milestone v1.23
/remove-lifecycle stale
I think what is keeping this open is that we are waiting to run down the deprecation clock so we can remove the old fields

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 27, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Jul 27, 2021
@k8s-ci-robot
Copy link
Contributor

@BenTheElder:
This request has been marked as suitable for new contributors.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/lifecycle frozen
/good-first-issue
#19264 (comment) should be pretty straightforward, this ought to be a decent starter issue.

You will need to remove the deprecated fields and there associated code, and perhaps double check if we've missed any documentation references. There also needs to be a note in the announcements that they are now removed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. labels Apr 19, 2022
@BenTheElder BenTheElder modified the milestones: v1.23, v1.25 Apr 19, 2022
@BenTheElder BenTheElder removed the wg/naming Categorizes an issue or PR as relevant to WG Naming. label Apr 19, 2022
@BenTheElder
Copy link
Member

wg/naming was dissolved.
Clearing assignees so it's clear that no-one is working on this.

If anyone is interested, please feel free to reach out to me or @chaodaiG, we can help answer any questions.

@vinayakshnd
Copy link

/assign

@harshitasao
Copy link
Contributor

Hi, @BenTheElder. If no one else is working, can I work on this issue?

@adhishmeena
Copy link

Hi , @chaodaiG Please share the update regarding this issue. I would like to work on this issue

@BenTheElder
Copy link
Member

Hi all, please feel free to work on this. You don't need to ask us for permission, but you might want to coordinate to avoid overlapping work.

I think this is almost done, but not fully removed yet https://cs.k8s.io/?q=blacklist&i=nope&files=&excludeFiles=&repos=kubernetes/test-infra

@harshitasao
Copy link
Contributor

@adhishmeena If that's okay with you, I'll go ahead and work on this.

@harshitasao
Copy link
Contributor

Hey @BenTheElder, I have made changes to all the required files. However,

  1. I'm not sure if I should remove the code or modify it here.

fq1

  1. Also, I'm not sure how to make changes to this function.

fq2

Can you please help me out?
Thank you.

@BenTheElder
Copy link
Member

  1. should be removed when the blacklist field is removed since there's no longer another flag to be exclusive with

  2. should remove the second line of the method appending.

@harshitasao
Copy link
Contributor

Thank you so much.

@harshitasao
Copy link
Contributor

@BenTheElder, can you please suggest where I should put the note in the ANNOUNCEMENTS.md
Thank you.

@BenTheElder
Copy link
Member

Under "Breaking Changes" since it is still a breaking change to remove them.
The entries are chronological with the current entry at the top. The prow versions are date + commit hash.

@harshitasao
Copy link
Contributor

harshitasao commented May 16, 2022

Got it. Thanks again.

@CypherpunkSamurai
Copy link

Hello Everyone, Is this issue still active?

I'm new to open-source and would like to contribute to this issue :)

@spiffxp
Copy link
Member Author

spiffxp commented Oct 27, 2022

/close
Only mentions left are in job configs or announcements describing the migration to allowlist/denylist

$ rg -il 'blacklist|whitelist'
config/jobs/kubernetes/sig-scalability/sig-scalability-presets.yaml
config/jobs/kubernetes-sigs/node-feature-discovery/node-feature-discovery-postsubmits.yaml
prow/ANNOUNCEMENTS.md

@k8s-ci-robot
Copy link
Contributor

@spiffxp: Closing this issue.

In response to this:

/close
Only mentions left are in job configs or announcements describing the migration to allowlist/denylist

$ rg -il 'blacklist|whitelist'
config/jobs/kubernetes/sig-scalability/sig-scalability-presets.yaml
config/jobs/kubernetes-sigs/node-feature-discovery/node-feature-discovery-postsubmits.yaml
prow/ANNOUNCEMENTS.md

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/prow Issues or PRs related to prow good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. sig/testing Categorizes an issue or PR as relevant to SIG Testing.
Projects
None yet
Development

No branches or pull requests