-
Notifications
You must be signed in to change notification settings - Fork 18
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
Validate plugins removed from hub before marking them stale #1301
Conversation
a33d3ca
to
afddf5d
Compare
16f9214
to
b9b3320
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manasaV3 thanks for working on this! Left a couple of comments.
@@ -341,6 +341,7 @@ module data_workflows_lambda { | |||
"GITHUB_CLIENT_ID" = local.github_client_id | |||
"GITHUB_CLIENT_SECRET" = local.github_client_secret | |||
"PLUGINS_LAMBDA_NAME" = local.plugins_function_name | |||
"ZULIP_CREDENTIALS" = local.zulip_credentials |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this re-enabling the zulip stream @manasaV3? Perhaps it would be good to do that in a separate PR once we've merged this and confirmed all is working as intended? Ideally tbh I would like to actually capture one of the events reported in #1725 before we re-enable.
Also regardless of that, we should flag to folks on zulip before we re-enable the channel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that we should decouple adding the Zulip credentials from this PR. Opened this PR for that.
We could let this run in prod for up to a month to see if we can catch the issue occurring. But, if we aren't able to, I think we could add Zulip notifications to let the community know that we are cautiously positive about having fixed the issue. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manasaV3 yep I think that sounds reasonable!
github_client_helper = GithubClientHelper(REPOSITORY) | ||
data = github_client_helper.get_file(FILE_NAME, "json") | ||
if not data: | ||
raise RuntimeError(f"Unable to fetch {FILE_NAME} from {REPOSITORY}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be worth logging here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that should be captured by the log here: https://github.com/chanzuckerberg/napari-hub/pull/1301/files#diff-1c206b5606711323610de2566d4543b35e6eac13a47494df807c5f47c6d1e74dL64
@@ -341,6 +341,7 @@ module data_workflows_lambda { | |||
"GITHUB_CLIENT_ID" = local.github_client_id | |||
"GITHUB_CLIENT_SECRET" = local.github_client_secret | |||
"PLUGINS_LAMBDA_NAME" = local.plugins_function_name | |||
"ZULIP_CREDENTIALS" = local.zulip_credentials |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manasaV3 yep I think that sounds reasonable!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manasaV3 looks good to me - just left a small comment about perhaps logging when the classifiers
file is unavailable
This PR has been included in release: v24.03.0, see the release notes. |
Description
Relates to: #1275
In cases where a plugin is being marked stale without a new version, we validate if the version is no longer active by validating it against
npe2api
repository'spublic/classifiers.json
.TODO:
Update the documentation to reflect it can now take upto 2 hours for a plugin to be removed from the site.