Dev plugin visibility #268

liu-ziyang · 2021-09-23T08:16:07Z

Warning: this PR has some major refactors of code structure to support future feature development.

(Q: Why refactor now? A: because of the plugin visibility feature change, we are reworking some of the underlying flow which resulted in complexity increase and fundamental data filter/manipulations, the main block grow past hundreds line of code without breaking down, and made it difficult to develop for future.)

In order to test the refactored code, I have been adding more unit tests along the way and try to regression test against
https://plugin-visibility-frontend.dev.imaging.cziscience.com/
It seems the environment have been working consistently comparing to prod site so far. (other than the newly introduced visibility flag, which is being tested now with napari-demo set to 'hidden' and it seems to be working correctly)

Just to be safe, I plan to leave this dev branch up a few days on regression testing and make sure the zulip messaging is also consistent with prod behavior

ps. did I mention cffconverter updated their library suddenly (8 hours ago when I was working on it) and supported APA? I added it in since it is a very small change

…yaml file

…ugin method

…luded_plugins.json

…et_plugins()

… in this PR - still trying to look into fixtures

…ari-hub into hidden-plugin-feature

klai95 · 2021-09-23T19:35:22Z

backend/s3.py

+    :param key: key to check in s3
+    :return: True iff cache exists
+    """
+    if bucket is None:


We can return False also if key is an empty string so we can short circuit the try-except clause?

true, though there is no use case for that yet

klai95 · 2021-09-23T19:41:15Z

backend/s3.py

+        return None
+
+
+def cache(content: Union[dict, list], key: str) -> Union[dict, list]:


Since we are using type Union for the parameter content, when do we expect content to be of type dict vs type list?

technically it doesn't care, it would cache the given content regardless of type, just that the hints are saying these are supported ones

klai95 · 2021-09-23T19:43:54Z

backend/s3.py

+    Cache the given content to the key location.
+
+    :param content: content to cache
+    :param key: key path in s3


Should we do an error check for the key parameter or an empty string check?

we don't have anything calling it with empty string right now

backend/pypi.py

klai95 · 2021-09-23T19:59:16Z

backend/pypi.py

+    """
+    version = get_attribute(plugin, ["info", "version"])
+
+    return {


we should include visibility here right?

nope, the visibility comes from github metadata, the logic is in model.py

klai95 · 2021-09-23T20:25:57Z

backend/model.py

+    :return: plugin metadata list
+    """
+    plugins_metadata = {}
+    with futures.ThreadPoolExecutor(max_workers=32) as executor:


How did we decide on max_worker=32?

it is more or less arbitrary, just trying to parallelize the work, it may not be the optimal setting but good enough so far

klai95 · 2021-09-23T21:28:50Z

backend/github.py

+        api_url = url.replace("https://github.com/",
+                              "https://api.github.com/repos/")
+        auth = None
+        if github_client_id is not None and github_client_secret is not None:


Would if None not in (github_client_id, github_client_secret): be more concise?

if we have more items I would be tempted to do so, but I personally found this more readable

klai95 · 2021-09-23T23:22:39Z

.happy/terraform/modules/ecs-stack/main.tf

@@ -37,7 +37,7 @@ locals {
  frontend_alb_zone     = try(local.secret[local.alb_key]["frontend"]["zone_id"], "")
  frontend_alb_dns      = try(local.secret[local.alb_key]["frontend"]["dns_name"], "")

-  slack_url = try(local.secret["slack_url"], "")
+  slack_url = try(local.secret["slack"]["url"], "")


Can you explain the context for this code change?

we updated the slack URL in the infra config, it does not allow set of string value in the secret so it was wrapped in a library

klai95 · 2021-09-24T07:37:32Z

backend/github.py

+        citations['APA'] = citation.as_apalike()
+    except ValueError:
+        # invalid CITATION.cff content
+        citations['citation'] = None


How about citations = dict.fromkeys(['citation', 'RIS', 'BibTex', 'APA'], None) in the except clause?

klai95 · 2021-09-24T07:49:33Z

backend/model.py

+    - cache/public-plugins.json (overwrite)
+    - cache/hidden-plugins.json (overwrite)
+    - cache/index.json (overwrite)
+    - cache/{plugin}/{version}.json (skip if exists)


Why do we skip if exists?

we don't reprocess metadata for plugin with known name and version because pypi metadata does not change

klai95 · 2021-09-24T07:51:58Z

backend/model.py

+        if plugin in plugins_metadata:
+            del (plugins_metadata[plugin])
+
+    if visibility_plugins['public']:


In the case where hidden plugins do exist, how come we are not calling notify_new_packages to notify if the conditions are true?

we do not send notifications for hidden plugins, these notifications are for everyone to know there is a plugin that is ready for public use

klai95 · 2021-09-24T07:57:02Z

backend/model.py

+    """
+    excluded_plugins = get_excluded_plugins()
+    for plugin, plugin_metadata in plugins_metadata.items():
+        if 'visibility' not in plugin_metadata:


if this condition is true, why do we not want to create the visibility key in plugin_metadata? Also, what would be the case in which this happens?

if visibility does not present we treat it as a public plugin, this would occur when the developer does not specify the plugin visibility, which is equivalent to what we are displaying now

klai95

LGTM!

…i sourcing clearer (#271) - adds field that describes functionality of Dev plugin visibility #268 - uses local svg for PyPI logo - revises wording of fields where we prioritize info in `.napari`

klai95 and others added 30 commits August 25, 2021 16:48

Step 1: Add reading and default behavior of visibility flag from the …

d871e65

…yaml file

Code Review Updates

c7428c1

STEP 2 and STEP 3

04cb04f

Test Error and Spacing Issue

4032b97

Added default behavior for white_list parameter in filter_excluded_pl…

3491e51

…ugin method

Refactored code

801a754

Updated s3 file and refactored parameter name

7855f6d

Added comments and refactored read/write operations in endpoints

3fb9c6e

Added code in update_index() to update and write plugins.json and exc…

1b0f499

…luded_plugins.json

Fixed test errors

5f66e01

Refactored methods and wrote comments

4e873f9

Modify test file to change version to visibility parameter

d4004da

In process of fixing test error from assert len(result) == 2 due to g…

3bdd831

…et_plugins()

Modified test_get_plugins() to have expected test code due to changes…

8883fc9

… in this PR - still trying to look into fixtures

Merge branch 'main' into hidden-plugin-feature

a9927ac

Some PR comment changes

068a532

Merge branch 'hidden-plugin-feature' of github.com:chanzuckerberg/nap…

7298de2

…ari-hub into hidden-plugin-feature

Refactored comment and added todo per pr comment

22c5c7f

update plugin visibility feature

b7ea3b8

correct boto3 api

3091cd9

correct visibility check

1efdd63

correct exclusion update

bd7ef33

ignore exclusion stats when visibility not present

d4bea94

add no value return

b608781

refactored code structure

0f6199a

address import structure

dac8d76

address import structure

95e6bc8

address project url override

b885e80

cache plugin metadata after build

fbfe761

correct github repo url

4598376

klai95 reviewed Sep 23, 2021

View reviewed changes

backend/pypi.py Outdated Show resolved Hide resolved

klai95 reviewed Sep 23, 2021

View reviewed changes

address comments

7eef38f

klai95 reviewed Sep 23, 2021

View reviewed changes

use slack url

22f725e

klai95 reviewed Sep 23, 2021

View reviewed changes

ziyang added 7 commits September 23, 2021 16:24

update s3 to use client only

67a45db

update s3 to use client only

4f55728

reinstate bucket path

6b960c0

correct s3 API

fd8b531

use bucket path to separate data in dev environment

6247a91

also verify empty url

07bf5ac

add alert for exceptions

e40a2a8

liu-ziyang requested a review from klai95 September 24, 2021 01:01

klai95 reviewed Sep 24, 2021

View reviewed changes

address comments on citation dict

367fce3

klai95 approved these changes Sep 24, 2021

View reviewed changes

liu-ziyang merged commit 67e3ca8 into main Sep 24, 2021

liu-ziyang deleted the dev-plugin-visibility branch September 24, 2021 21:14

liu-ziyang linked an issue Sep 24, 2021 that may be closed by this pull request

hidden plugins #247

Closed

This was referenced Sep 28, 2021

updates plugin dev docs to add "visibility" field and make github/pypi sourcing clearer #271

Merged

hidden plugins #247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dev plugin visibility #268

Dev plugin visibility #268

liu-ziyang commented Sep 23, 2021 •

edited

Loading

klai95 Sep 23, 2021 •

edited

Loading

liu-ziyang Sep 23, 2021

klai95 Sep 23, 2021

liu-ziyang Sep 23, 2021

klai95 Sep 23, 2021

liu-ziyang Sep 23, 2021

klai95 Sep 23, 2021

liu-ziyang Sep 23, 2021

klai95 Sep 23, 2021

liu-ziyang Sep 23, 2021

klai95 Sep 23, 2021

liu-ziyang Sep 23, 2021

klai95 Sep 23, 2021

liu-ziyang Sep 23, 2021

klai95 Sep 24, 2021 •

edited

Loading

klai95 Sep 24, 2021

liu-ziyang Sep 24, 2021 •

edited

Loading

klai95 Sep 24, 2021

liu-ziyang Sep 24, 2021 •

edited

Loading

klai95 Sep 24, 2021

liu-ziyang Sep 24, 2021 •

edited

Loading

klai95 left a comment

		return None


		def cache(content: Union[dict, list], key: str) -> Union[dict, list]:

Dev plugin visibility #268

Dev plugin visibility #268

Conversation

liu-ziyang commented Sep 23, 2021 • edited Loading

klai95 Sep 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klai95 Sep 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liu-ziyang Sep 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liu-ziyang Sep 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liu-ziyang Sep 24, 2021 • edited Loading

Choose a reason for hiding this comment

klai95 left a comment

Choose a reason for hiding this comment

liu-ziyang commented Sep 23, 2021 •

edited

Loading

klai95 Sep 23, 2021 •

edited

Loading

klai95 Sep 24, 2021 •

edited

Loading

liu-ziyang Sep 24, 2021 •

edited

Loading

liu-ziyang Sep 24, 2021 •

edited

Loading

liu-ziyang Sep 24, 2021 •

edited

Loading