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

feat(hex): use registry v2 for packages rather than hex.pm http API #29756

Conversation

bryannaegele
Copy link
Contributor

Changes

  • use hex registry v2 for package information
  • use hex.pm package API for supplemental package metadata when package is hosted there.
  • support registryAliases for private repo/registry mapping
  • run mix.repo add in mix manager for private repos

Context

#29622

This changes the hex datasource to use hex registry v2 for package management. Hex registries are not required to implement an http API like hex.pm has, so the prior implementation prevented updating of packages in private repos such as oban pro. Additionally, due to the way mix works when updating dependencies, mix.lock files could not be updated when such a dependency existed and the mix manager ran mix deps.update <<updated packages>>.

In order to keep as much parity with the prior implementation, hex.pm is used for supplemental package metadata where possible.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@bryannaegele bryannaegele marked this pull request as draft June 19, 2024 00:42
@bryannaegele bryannaegele changed the title Use hex registries for packages rather than hex.pm http API Use hex registry v2 for packages rather than hex.pm http API Jun 19, 2024
@bryannaegele bryannaegele changed the title Use hex registry v2 for packages rather than hex.pm http API feat/Use hex registry v2 for packages rather than hex.pm http API Jun 19, 2024
@bryannaegele bryannaegele changed the title feat/Use hex registry v2 for packages rather than hex.pm http API feat(hex) use registry v2 for packages rather than hex.pm http API Jun 19, 2024
@bryannaegele bryannaegele changed the title feat(hex) use registry v2 for packages rather than hex.pm http API feat(hex): use registry v2 for packages rather than hex.pm http API Jun 19, 2024
@bryannaegele
Copy link
Contributor Author

Leaving this as a draft until there's an answer for this #29622 (comment)

I am no TS expert, so please feel free to jump in and fix up whatever can be done better.

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

reduce fixtures, we don't need all versions for testing, ~3 are enough. ensure the tgz file is at minimum too

lib/modules/datasource/hex/package.spec.ts Outdated Show resolved Hide resolved
lib/modules/datasource/hex/package.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/mix/artifacts.ts Outdated Show resolved Hide resolved
lib/modules/datasource/hex/signed.ts Show resolved Hide resolved
@bryannaegele
Copy link
Contributor Author

reduce fixtures, we don't need all versions for testing, ~3 are enough. ensure the tgz file is at minimum too

Is this strictly necessary? The tgz has the registry version of the package wrapped in a signed wrapper. I don't have any way to create a version with less releases in it that would have a valid signature.

I can try to find a different package as an example but this is one of the rare ones with retirements.

@bryannaegele
Copy link
Contributor Author

Mix changes split to #29775

@bryannaegele bryannaegele marked this pull request as ready for review June 24, 2024 17:22
@bryannaegele bryannaegele changed the base branch from main to v38 July 8, 2024 19:43
@bryannaegele bryannaegele changed the base branch from v38 to main July 8, 2024 19:43
@bryannaegele bryannaegele force-pushed the feat/29622-hex-registry-support branch from 6f0e4ee to 97ceb1d Compare July 8, 2024 19:55
@bryannaegele bryannaegele force-pushed the feat/29622-hex-registry-support branch from 453dc5a to 2e0ea0a Compare July 26, 2024 16:16
@rarkins rarkins changed the title feat(hex)!: use registry v2 for packages rather than hex.pm http API feat(hex): use registry v2 for packages rather than hex.pm http API Jul 26, 2024
@bryannaegele
Copy link
Contributor Author

Alright. Made the following requested changes:

  • remove codegen from CI and setup
  • commit generated files
  • add readme with info on how to generate the proto files
  • renamed script name in package per @zharinov's suggestion

I have kept the generation script in tools so that if anyone should need to regenerate the files, it's as simple as possible.

@esambo
Copy link

esambo commented Aug 13, 2024

@bryannaegele FYI, you have merge conflicts

@bryannaegele
Copy link
Contributor Author

@bryannaegele FYI, you have merge conflicts

Are y'all ready to merge this and the other PR? I'd like to not keep resolving the conflicts unless it's going to get merged.

@rarkins rarkins requested a review from viceice August 24, 2024 06:19
# Conflicts:
#	package.json
#	pnpm-lock.yaml
lib/modules/datasource/hex/index.ts Outdated Show resolved Hide resolved
override readonly releaseTimestampSupport = true;
override readonly releaseTimestampNote =
'The release timestamp is determined the `inserted_at` field in the results.';
override readonly releaseTimestampSupport = false;
Copy link
Member

Choose a reason for hiding this comment

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

that's a bad step back. isn't it possible to still support release time stamps?

this is really a breaking change and a new show stopper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Timestamps are not a piece of metadata in the registry metadata. They are only a piece of metadata that hex.pm supports via its HTTP API which is not a requirement to run a registry.

Available fields.

inserted_at field is preserved but that field is only available for a package served from hexpm.

Can you explain why it's a showstopper? I understand it's a potentially breaking change but the original implementation was technically incorrect. I'm not sure how the releaseTimestampSupport is used. If we set it to true and the value isn't available for some packages, what would happen?

lib/modules/datasource/hex/index.ts Outdated Show resolved Hide resolved
lib/modules/datasource/hex/index.ts Outdated Show resolved Hide resolved
lib/modules/datasource/hex/index.ts Outdated Show resolved Hide resolved
lib/modules/datasource/hex/index.ts Outdated Show resolved Hide resolved
lib/modules/datasource/hex/index.ts Outdated Show resolved Hide resolved
lib/modules/datasource/hex/index.ts Outdated Show resolved Hide resolved
lib/modules/datasource/hex/index.ts Outdated Show resolved Hide resolved
function generateProto(protos_path: string, file: string): Promise<string> {
return new Promise((resolve, reject) => {
console.log(`Current directory: ${process.cwd()}`);
fs.readdirSync(process.cwd()).forEach((file) => {
Copy link
Member

Choose a reason for hiding this comment

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

use fs/promises api

@bryannaegele
Copy link
Contributor Author

Handing to Maintainers to Finish

I leave this PR and the Mix PR #29775 to the maintainers to complete. My limited time is better spent on my maintainer duties elsewhere.

These PRs are required to Elixir/Hex support for companies to adopt Renovate in non-trivial setups. First-order support for the hex registry is necessary to support the Hex package manager. This was unfortunately missed during original implementation in the good faith effort to support Hex/Mix.

Reasons for the handoff:

  • compatibility concerns and solutions are best left to maintainers
  • maintainer expertise on testing techniques employed in the project will provide better coverage
  • strong opinions on code style, organization, and nits are best resolved by codeowners

Three months and >100 hours greatly surpasses the level of effort I thought this would take when I volunteered to investigate and take a stab.

Renovate is a great project. I hope you can find resources to complete the work @rarkins so we can consider using it for Elixir projects again someday.

Thanks

#29622

@rarkins rarkins closed this Sep 4, 2024
@rarkins
Copy link
Collaborator

rarkins commented Sep 4, 2024

@bryannaegele thanks for your efforts. I've closed this if you're unable to continue and hope someone can pick it up in future.

@bryannaegele
Copy link
Contributor Author

@viceice should have enough to wrap up. I'd hate to see all this work go to waste.

@zharinov zharinov self-assigned this Sep 4, 2024
@zharinov
Copy link
Collaborator

zharinov commented Sep 4, 2024

@bryannaegele Thanks for your effort, it definitely isn't easy one 🤝

I'll take a look if I could reuse it as series of smaller changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change, requires major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants