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

fix: adds alternate lookup if base lookup fails #896

Merged
merged 4 commits into from
Jul 6, 2023

Conversation

holtjma
Copy link
Contributor

@holtjma holtjma commented May 25, 2023

This is a first pass attempt at fixing #895.

Main changes are updating the GithubRelease hoster to look for alternate paths via the GitHub API. The API sends a JSON response (example: https://api.github.com/repos/PacificBiosciences/HiPhase/releases) that contains each release along with a list of assets. The assets are searched for files that match the provides pattern. Currently, this first pass puts this as a backup to the main one (which I think is always failing now). I also did not update the tests, I think someone may have a better idea of how to do that. Happy to help push this further, but wanted to get the ball rolling.

@holtjma holtjma changed the title adds alternate lookup if base lookup fails fix: adds alternate lookup if base lookup fails May 25, 2023
@holtjma
Copy link
Contributor Author

holtjma commented May 25, 2023

Not sure what those failures are, they seem to be related to DESeq2. I think it's unrelated to the changes here, but I could be wrong.

@holtjma
Copy link
Contributor Author

holtjma commented May 26, 2023

@bioconda/core

Not sure who to ping here, but hoping for feedback on the PR content and debugging that test (seemingly related to bioconductor).

@dpryan79
Copy link
Contributor

Wow, yeah that DESeq2 failure is definitely not your fault, I suspect bioconductor has removed the file.

@dpryan79
Copy link
Contributor

@johanneskoester @mbargull Any thoughts on this PR?

@holtjma
Copy link
Contributor Author

holtjma commented Jun 1, 2023

@johanneskoester @mbargull @dpryan79

Circling back on this, any thoughts on the functional changes and/or fixing the test? Happy to look into making the changes, just need to know what's missing here.

Copy link
Contributor

@johanneskoester johanneskoester left a comment

Choose a reason for hiding this comment

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

Looks good to me. I've updated with the master branch, which should fix the test issue with deseq if I remember correctly.

@holtjma
Copy link
Contributor Author

holtjma commented Jun 20, 2023

@johanneskoester I fixed that previous error by just bumping the versions (assuming this is fine). However, there's some new error in the OSX tests (this appears to have been added after the master branch merge). I took a quick look, but it's not obvious to me what's happening there.

@dpryan79 dpryan79 merged commit 17a1475 into bioconda:master Jul 6, 2023
dpryan79 pushed a commit that referenced this pull request Jul 6, 2023
@holtjma holtjma deleted the fix_autobump_githubreleases branch July 6, 2023 23:19
johanneskoester pushed a commit that referenced this pull request Aug 16, 2023
This is basically a patch to fix a new issue (described here:
bioconda/bioconda-recipes#42176) that was
introduced by myself here:
#896.

The primary cause is that the GitHub API is rate limited (which I was
not aware of), so autobump basically blows through the limits quickly
and then subsequent calls automatically fail. Of course, this means that
the tools we were hoping to see get autobumped are not.

This patch basically removes the original patch (which uses a single
GitHub API call) and replaces it with an approach that basically
"expands" the release webpage to find the assets. See
#895 for more details
on how the URLs are masked and the expansion pattern.

Couple other notes:
* I added an `IncludeFragmentParser` that is basically identical to the
`HrefParser` except it is looking for tags with this pattern:
`<include-fragment src="http://url/of/interest" ...>`
* The general expansion logic is:
  * Pull the full release page
  * Search for `<include-fragment ...>` tags
  * Iteratively pull these looking for the main link URL of interest 
  * Break the above iteration early if the current version is identified
* This should no longer use the GitHub API, I'm unaware of other rate
limiters for these URLs (🤞that none are hidden...)

Pinging @dpryan79 and @johanneskoester as you both reviewed the last
related PR. Sorry that the previous PR injected a rate limit issue!

EDIT: I forgot, I have tested these on both `hiphase` and `pbfusion`
recipes, the diff of the results are here and match our current
expectations:
```
(bioconda)$ git diff
diff --git a/recipes/hiphase/meta.yaml b/recipes/hiphase/meta.yaml
index 38b939bf9f..b5d2503597 100644
--- a/recipes/hiphase/meta.yaml
+++ b/recipes/hiphase/meta.yaml
@@ -1,6 +1,6 @@
 {% set name = "hiphase" %}
-{% set version = "0.10.0" %}
-{% set hiphase_sha256 = "e22d3530a395bd218bd4634537f5b3c31a82cd8155849291c69d81c48f0c58fe" %}
+{% set version = "0.10.2" %}
+{% set hiphase_sha256 = "2a20fc437ab5c0f2437246e8a36e7590c68f5ae5caa0cecb8e85a371f5357c6c" %}
 
 package:
   name: {{ name }}
diff --git a/recipes/pbfusion/meta.yaml b/recipes/pbfusion/meta.yaml
index 781ada3413..ca69e87d0f 100644
--- a/recipes/pbfusion/meta.yaml
+++ b/recipes/pbfusion/meta.yaml
@@ -1,6 +1,6 @@
 {% set name = "pbfusion" %}
-{% set version = "0.2.2" %}
-{% set pbfusion_sha256 = "3390adbeb8550265fcf4fad89dad80740cc368e96d10f3b23bb19ef001ffd081" %}
+{% set version = "0.3.0" %}
+{% set pbfusion_sha256 = "ef9767af7e82bd1e664711e7521d7a8370a41f101cccf1db4a58895d9f88934c" %}
 
 package:
   name: {{ name }}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants