-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
feat(rubygems): fallback to info when version fails #7323
feat(rubygems): fallback to info when version fails #7323
Conversation
when the version endpoint fails use the single version from the info endpoint. This occurs for example when using artifactory.
not quite sure why macos fails on install when i didnt even changed the package.json |
and im not quite sure if i need to wrap the await into a try catch because i only worked with promises and observables until now. |
@rarkins not quite sure how to tag you in this here i'm more of a gitlab guy. Please don't feel annoyed by this would just be happy to get feedback. |
just found #7111 @micheelengronne maybe we could somehow combine our efforts in this area? |
but the problem in my case seems to be that artifactory doesnt emit the dependencies as json. Instead it seems to be some sort of file that i dont know how to open to properly read it. see last request on here this site: |
So Artifactory does have an endpoint that lists all available versions? |
i just found out that you can misuse the dependencies endpoint for this 😅 but the problem here is that it is in this marshalled array format and its a file you need to download. not JSON or XML. Just found it because I found the MR mentioned above. |
Thanks @henrysachs I dont' currently have time to finish my MR. It was intended to be used with the Gemstash repository. Feel free to take what you need from it. |
I think there's value in falling back to the info endpoint "latest" result as-is and then adding additional versions retrieval after. |
of course. Maybe i can do a seperate pr for that. But i currently dont have an idea how to properly parse this ruby marshalled array. |
So just to come back to this one what do you think about it? |
lib/datasource/rubygems/get.ts
Outdated
@@ -52,7 +52,7 @@ export async function getDependency( | |||
|
|||
const versions = (await fetch(dependency, registry, VERSIONS_PATH)) || []; |
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.
Doesn't this request throw ? If so then everything below it is not executed.
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 thought it doesnt because of the || if not than i can add a try catch around it
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.
just to be sure a try catch would look like the following right? I can implement this if you want me to.
let versions = []
try {
versions = await fetch(dependency, registry, VERSIONS_PATH);
} catch {
// all of my code
}
so i just catched the error and moved some code to match the flow better. Now i got everything working and received a pull request for this repo: https://github.com/henrysachs/ruby-test-1337/pull/8 also found out that artifactory is just pulling "through" to the version endpoint if its just a mirrored dependency so i had to manually tweak the version path that it erros on purpose. |
when everything is fine i would like to rebase to squash all commits into 1 and add myself to the contributors 😅 |
You don't need to squash, we will squash the pr anyways. So just add yourself to the contributed list. 🙃 |
lib/datasource/rubygems/get.ts
Outdated
rubyPlatform: info.platform, | ||
releaseTimestamp: null, | ||
rubyVersion: null, | ||
rubygemsVersion: '\u003e= 0', |
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.
What is the meaning of this value?
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.
it was the default value from the versions endpoint its basically >= 0
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.
but we can remove it I just wanted to match the signature from the "default" case
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.
Yes, remove it
Co-authored-by: Rhys Arkins <[email protected]>
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.
Needs tests to get coverage back to 100%, also run yarn lint-fix
never done something like that is there somewhere you can point me to? |
Test files are colocated with source files and you can find some existing rubygems tests already that you can duplicate then modify |
added tests and run yarn lint fix. |
What happens after the approval? 😅 |
It needs to be up to date and pass all tests before we merge |
Thanks! So happy for your help to get me through this. 🎉 |
🎉 This PR is included in version 23.34.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
when the version endpoint fails use the single version from the info endpoint.
This occurs for example when using artifactory.
Closes #7320