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(mix): Support private hex repos/registries #29775

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

bryannaegele
Copy link
Contributor

Changes

This adds support to the mix manager to add private hex repos/registries via registryAliases for mapping and hostRules for authentication.

Context

This is the companion to #29756 and dependent on that change. Splitting the manager and datasource work per request in #29756. When combined, this will enable support for private repos/registries such as oban pro.

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

Tested in combination with #29756. This will not fully work without that code.

@bryannaegele bryannaegele marked this pull request as draft June 20, 2024 15:27
@bryannaegele bryannaegele marked this pull request as draft June 20, 2024 15:27
@bryannaegele bryannaegele marked this pull request as ready for review June 24, 2024 17:23
@HonkingGoose HonkingGoose mentioned this pull request Jul 25, 2024
7 tasks
@epinault

This comment has been minimized.

lib/modules/manager/mix/artifacts.ts Outdated Show resolved Hide resolved
lib/modules/manager/mix/artifacts.ts Outdated Show resolved Hide resolved
lib/modules/manager/mix/artifacts.ts Outdated Show resolved Hide resolved
lib/modules/manager/mix/artifacts.ts Outdated Show resolved Hide resolved
@bryannaegele
Copy link
Contributor Author

Feedback should be addressed 👍🏻

@rarkins rarkins requested a review from viceice August 21, 2024 09:38
file: { type: 'addition', path: 'mix.lock', contents: 'New mix.lock' },
},
]);
expect(execSnapshots).toMatchObject([
Copy link
Member

Choose a reason for hiding this comment

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

i don't see a test for the mix repo.add ... command

Copy link
Contributor Author

@bryannaegele bryannaegele Aug 21, 2024

Choose a reason for hiding this comment

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

The test is to check that we don't do that in this case. does not add registries configured in registryAliases but no token in hostRules

Copy link
Member

Choose a reason for hiding this comment

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

then add a test for it

@@ -75,26 +78,50 @@ export async function updateArtifacts({

for (const { packageName } of updatedDeps) {
if (packageName) {
const [, organization] = packageName.split(':');
const [, organization, ,] = packageName.split(':');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const [, organization, ,] = packageName.split(':');
const [, organization] = packageName.split(':');

not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand the suggestion. This wants to extract the second value in whatever is returned. The packageName when split can be 2 or 3 elements in length. The suggestion only works when array of length 2 is returned.

Copy link
Member

Choose a reason for hiding this comment

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

no, the split returns all elements, but the array destruction will only return the first two and ignore the rest.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment


return acc;
}, [] as string[]);
// istanbul ignore if: no good way to test
Copy link
Member

Choose a reason for hiding this comment

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

no, why? you can mock hostrule find properly

return packageName;
}

const [, repoName, ,] = packageName.split(':');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const [, repoName, ,] = packageName.split(':');
const [, repoName] = packageName.split(':');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand the suggestion. This wants to extract the second value in whatever is returned.

Comment on lines +51 to +53
registryAliases: {
oban: 'https://getoban.pro/repo',
},
Copy link
Member

Choose a reason for hiding this comment

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

this should be scoped to mix manager, otherwise it will interferre with other managers using registry aliases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So there needs to be a further nesting of the object? I didn't see any examples of that in other managers

Comment on lines +11 to +13
registryAliases: {
hexpm: 'https://hex.pm',
},
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the URL for the hexpm registry, which is the default, so we put it into the default config. This is merged with anything the user has supplied, giving us a complete lookup of the URLs. This has an added benefit that if a company wished to host an internal proxy registry, no updates are needed to their mix files; they have a way to point someplace else.

Copy link
Member

Choose a reason for hiding this comment

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

you should not add a default url, the datasource has it already, so only set registry url if it's not the default

if (requirement?.startsWith('==')) {
dep.currentVersion = requirement.replace(regEx(/^==\s*/), '');
}

if (organization) {
dep.packageName = `org:${organization}:${app}`;
Copy link
Member

Choose a reason for hiding this comment

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

this is used by the hex datasource, so needs changes there too

const [hexPackageName, organizationName] = packageName.split(':');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is in the other PR. You requested to split them so neither PR will work on its own

Copy link
Member

Choose a reason for hiding this comment

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

then don't change package name in this PR, do the package name change in a separate refactor PR

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.

None yet

4 participants