Closed svonworl closed 2 months ago
Attention: Patch coverage is 76.47059%
with 4 lines
in your changes are missing coverage. Please review.
Project coverage is 72.88%. Comparing base (
7347da6
) to head (6cec07d
).
Files | Patch % | Lines |
---|---|---|
...n/java/io/dockstore/webservice/api/SyncStatus.java | 66.66% | 2 Missing :warning: |
.../dockstore/webservice/resources/EntryResource.java | 77.77% | 0 Missing and 2 partials :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Lots of conflicting feedback, took a crack at getting closer to consensus. In the end, I:
syncStatus
. This dovetails with the notion that "sync" means different things to different people, but broadly, could encompass the act/process/result of automatically updating an entry, and thus, can return other related info in the future, or refer to other methods of automatically, should we someday implement them.gitHubAppInstalled
as a boolean. If we determine that we really need a three-phase TRUE/FALSE/UNKNOWN result, we can add it later.Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
86.2% Coverage on New Code
0.0% Duplication on New Code
Description This PR adds an
isSyncing
endpoint to the webservice that indicates whether or not the specified Entry is being automatically synced (currently, via our GitHub app).Regarding the design and functionality of this endpoint, there were a number of different options as to complexity and effectiveness:
UNINSTALL
LambdaEvent (https://github.com/dockstore/dockstore/pull/5852) to tell if the GitHub App was uninstalled (on or after the date we first deploy theUNINSTALL
logging).In the end, I decided to go with the most basic approach, which is to conclude that an entry is not syncing iff the latest
LambdaEvent
for the entry's repo is anUNINSTALL
. All the other approaches were much more involved. Sure, they would give us more visibility for older entries, but everyone I consulted with seemed to [slightly grudgingly] agree that the basic solution is good enough. I would love it to be perfect and give us useful information about preexisting uninstalls, but I don't think the engineering cost justifies the payoff. As it is, it's a quick little addition that will help us diagnose new issues.Access to the endpoint is limited to entry owners and admins.
Note that since
LambdaEvents
can be delivered to the webservice out of order, and we don't log the GitHub creation date (because in general, the webhook events don't include one), it's theoretically possible that anUNINSTALL
will get logged after events that were actually created later, and cause the endpoint to report that an entry is not syncing. Similar issues can happen if we fail to log an event that directly follows anUNINSTALL
for whatever reason. Such cases should be fairly rare, and will tend to heal on their own, because when the next non-UNINSTALL
event gets logged, the endpoint will report that the entry is syncing.There will be a companion UI PR, coming soon. Currently, the plan is to display some sort of warning on the private .dockstore.yml-based entry page if the entry is not syncing, and leave the UI as-is otherwise.
Review Instructions Register an entry in a test GitHub repository via the GitHub App / .dockstore.yml path. Query this endpoint and confirm that it indicates that the entry is syncing. Uninstall the GitHub App from the repo and wait five minutes. Query this endpoint and confirm that it indicates that the entry is not syncing. Reinstall the GitHub App on the repo and wait five minutes. Query this endpoint and confirm that it indicates the the entry is syncing.
Issue https://ucsc-cgl.atlassian.net/browse/DOCK-2312 https://github.com/dockstore/dockstore/issues/5331
Security and Privacy
No concerns.
e.g. Does this change...
Please make sure that you've checked the following before submitting your pull request. Thanks!
mvn clean install
@RolesAllowed
annotation