-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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(datasource): add debian datasource #30071
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for your contribution, and the well documented functionality!
This is my first pass as there is a lot going on here and we have to be sure that the caching is setup efficiently.
Would this work for every APT repository?
We should start simple and then start to optimize. |
Thx for the initial review, I will fix the findings soon.
should work with every apt repository, which follows the https://wiki.debian.org/DebianRepository standard. |
Co-authored-by: Sebastian Poxhofer <secustor@users.noreply.github.com>
…t/debian-datasource
The |
Co-authored-by: Sebastian Poxhofer <secustor@users.noreply.github.com>
Can we add this though or something speaking against it? |
Add it, the only downside is for scenarios in which the metadata conflicts. |
* @param config - Configuration for fetching releases. | ||
* @returns The release result if the package is found, otherwise null. | ||
*/ | ||
async getReleases({ |
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.
@zharinov should we enable the cachable property to true? so this call is also cached?
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 is already cached in getPackageIndex, so it would be redundant
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.
The @cache
decorator is the more functional alternative, e.g. you can set the TTL
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.
We will get cache bust for each configuration option and their order in the registryUrl though.
The current implementation checks cache after breaking down this options.
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 meant its already done here.
@cache({
namespace: `datasource-${DebDatasource.id}`,
key: (componentUrl: string) => componentUrl,
})
async getPackageIndex(
componentUrl: string,
): Promise<Record<string, PackageDescription[]>> {
const { extractedFile, lastTimestamp } =
await this.downloadAndExtractPackage(componentUrl);
return await this.parseExtractedPackageIndex(extractedFile, lastTimestamp);
}
No async operations happen in getReleases, if the cache of getPackageIndex is not stale.
but I still added this.
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.
let me revert the change again, github seems bugging and not loading latest the conversations 😅.
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.
Pleas add cache deocator to this function, so the aggregated result is cached too. (perf)
…enovate into feat/debian-datasource
* @param config - Configuration for fetching releases. | ||
* @returns The release result if the package is found, otherwise null. | ||
*/ | ||
async getReleases({ |
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.
Pleas add cache deocator to this function, so the aggregated result is cached too. (perf)
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
Changes
Context
Resolves: #7041, #24906
Concept
Based on the discussion and the reused implementation (see the related issue for more details):
Evaluation of Provided or Default Registry URL
https://ftp.debian.org/debian?release=bullseye&components=main,contrib&binaryArch=amd64
is evaluated to:.../debian/dists/bullseye/main/binary-amd64
.../debian/dists/bullseye/contrib/binary-amd64
Check for Existing Compressed Package
Packages.gz
file has already been downloaded and extracted:Iterate through Package Index
Return relevant release information
Documentation (please check one with an [x])
included documentation for new datasource
How I've tested my work (please select one)
I have verified these changes via:
Real Test
Open Points
Does the current caching implementation for the datasource need to be reworked? Can the Renovate team provide a concept/details for this? (e.g., invalidation of the compressed file based on its timestamp)
Is the current approach generic enough? Are there any points that were not considered?
If significant rework is required, please let me know in advance.