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

Replace 'whitelist' and 'blacklist' language #36527

Merged
merged 20 commits into from
Aug 25, 2020

Conversation

neha-ojha
Copy link
Member

This is a rebased version of #35628 with additional changes.

@asettle
Copy link
Contributor

asettle commented Aug 10, 2020

Hi @neha-ojha - whilst I am in agreement with the fundamental changes here, I think further thought should go into the choices of language. Reposting my comment/suggestion that was in the other PR:

  • "Ignore" can be interpreted as both "ignore this, let them in" or "ignore this, do not let them in".
  • To use 'ignorelist' as a verb is strange, we would recommend 'allow'. To use 'ignorelist' as a noun has a wrong meaning, one can think
    that 'ignorelisted' members will be ignore, when the contrary is true

@@ -5,7 +5,7 @@ tasks:
# tests may leave mgrs broken, so don't try and call into them
# to invoke e.g. pg dump during teardown.
wait-for-scrub: false
log-whitelist:
log-ignorelist:
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this has already been discussed and settled, but I still find that ignorelist-blocklist lacks the clarity of allowlist-denylist (e.g.: as in kubernetes/kubernetes#90277).

@neha-ojha
Copy link
Member Author

Hi @neha-ojha - whilst I am in agreement with the fundamental changes here, I think further thought should go into the choices of language. Reposting my comment/suggestion that was in the other PR:

  • "Ignore" can be interpreted as both "ignore this, let them in" or "ignore this, do not let them in".
  • To use 'ignorelist' as a verb is strange, we would recommend 'allow'. To use 'ignorelist' as a noun has a wrong meaning, one can think
    that 'ignorelisted' members will be ignore, when the contrary is true

Hi @asettle, I totally understand where you are coming from but I believe the essence of this change resonates with you. ignorelist in this context refers to list of strings that we want tests to ignore when looking for real bugs/issues and if it helps we can make that very clear by documenting it. In hindsight, perhaps, log-whitelist wasn't the right term for it in the first place.

@neha-ojha
Copy link
Member Author

changelog:

  • resolved conflict in src/client/Client.cc and src/client/Client.h

@batrick
Copy link
Member

batrick commented Aug 13, 2020

@LenzGr how would you like to handle these:

src/pybind/mgr/dashboard/.pylintrc:# Add files or directories to the blacklist. They should be base names, not
src/pybind/mgr/dashboard/.pylintrc:# Add files or directories matching the regex patterns to the blacklist. The
src/pybind/mgr/dashboard/controllers/auth.py:        JwtManager.blacklist_token(token)
src/pybind/mgr/dashboard/frontend/.stylelintrc:    "scss/at-import-partial-extension-blacklist": null,
src/pybind/mgr/dashboard/frontend/tslint.json:    "import-blacklist": [true, "rxjs/Rx", {"@angular/core/testing": ["async"]}],
src/pybind/mgr/dashboard/services/auth.py:    JWT_TOKEN_BLACKLIST_KEY = "jwt_token_black_list"
src/pybind/mgr/dashboard/services/auth.py:            if not JwtManager.is_blacklisted(dtoken['jti']):
src/pybind/mgr/dashboard/services/auth.py:    def blacklist_token(cls, token):
src/pybind/mgr/dashboard/services/auth.py:        blacklist_json = mgr.get_store(cls.JWT_TOKEN_BLACKLIST_KEY)
src/pybind/mgr/dashboard/services/auth.py:        if not blacklist_json:
src/pybind/mgr/dashboard/services/auth.py:            blacklist_json = "{}"
src/pybind/mgr/dashboard/services/auth.py:        bl_dict = json.loads(blacklist_json)
src/pybind/mgr/dashboard/services/auth.py:        mgr.set_store(cls.JWT_TOKEN_BLACKLIST_KEY, json.dumps(bl_dict))
src/pybind/mgr/dashboard/services/auth.py:    def is_blacklisted(cls, jti):
src/pybind/mgr/dashboard/services/auth.py:        blacklist_json = mgr.get_store(cls.JWT_TOKEN_BLACKLIST_KEY)
src/pybind/mgr/dashboard/services/auth.py:        if not blacklist_json:
src/pybind/mgr/dashboard/services/auth.py:            blacklist_json = "{}"
src/pybind/mgr/dashboard/services/auth.py:        bl_dict = json.loads(blacklist_json)
src/pybind/mgr/dashboard/tests/test_rest_tasks.py:# pylint: disable=blacklisted-name
src/pybind/mgr/dashboard/tests/test_tools.py:# pylint: disable=blacklisted-name

@batrick
Copy link
Member

batrick commented Aug 13, 2020

@neha-ojha there's also the use of "whiteout" term within the osd. Can you think of a better name for that?

liewegas and others added 20 commits August 24, 2020 19:53
Signed-off-by: Sage Weil <sage@newdream.net>
Signed-off-by: Neha Ojha <nojha@redhat.com>
Signed-off-by: Sage Weil <sage@newdream.net>
Signed-off-by: Sage Weil <sage@newdream.net>
Signed-off-by: Sage Weil <sage@newdream.net>
Signed-off-by: Sage Weil <sage@newdream.net>
I would have expected ENOSYS or EOPNOTSUPP or similar, but the mon returns
EINVAL on an unrecognized command

Signed-off-by: Sage Weil <sage@newdream.net>
Signed-off-by: Sage Weil <sage@newdream.net>
Signed-off-by: Neha Ojha <nojha@redhat.com>
…klist

Signed-off-by: Neha Ojha <nojha@redhat.com>
Signed-off-by: Neha Ojha <nojha@redhat.com>
Signed-off-by: Neha Ojha <nojha@redhat.com>
Signed-off-by: Neha Ojha <nojha@redhat.com>
Signed-off-by: Neha Ojha <nojha@redhat.com>
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
Signed-off-by: Neha Ojha <nojha@redhat.com>
Signed-off-by: Neha Ojha <nojha@redhat.com>
Signed-off-by: Ernesto Puerta <epuertat@redhat.com>
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
@neha-ojha
Copy link
Member Author

changelog:

  • resolved conflict in PendingReleaseNotes

@neha-ojha
Copy link
Member Author

jenkins test dashboard

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.