Skip to content

Conversation

@oschwald
Copy link

@oschwald oschwald commented Jan 26, 2026

Changes

This pull request adds support for the mise github backend. This is similar to the ubi backend except the version constraints allowed for it seem to be more limited.

Context

Please select one of the following:

  • This closes an existing Issue, Closes: #
  • This doesn't close an Issue, but I accept the risk that this PR may be closed if maintainers disagree with its opening or implementation

AI assistance disclosure

Did you use AI tools to create any part of this pull request?

Please select one option and, if yes, briefly describe how AI was used (e.g., code, tests, docs) and which tool(s) you used.

  • No — I did not use AI for this contribution.
  • Yes — minimal assistance (e.g., IDE autocomplete, small code completions, grammar fixes).
  • Yes — substantive assistance (AI-generated non‑trivial portions of code, tests, or documentation).
  • Yes — other (please describe):

I used Claude Code to help with this PR. In particular, it helped significantly with the tests and making sure everything outside of the core logic was updated appropriately.

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

The public repository:

I tested it against our internal GitHub Enterprise Server instance.

@cla-assistant
Copy link

cla-assistant bot commented Jan 26, 2026

CLA assistant check
All committers have signed the CLA.

@RahulGautamSingh RahulGautamSingh self-requested a review January 26, 2026 19:35
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.

otherwise LGTM

}
} else if (!hasVPrefix) {
// Default: strip 'v' prefix if current version doesn't have it
extractVersion = '^v?(?<version>.+)';
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 like that, it causes performance loose when it's not needed. it should be configured explicit if needed. 🤔

Copy link
Author

Choose a reason for hiding this comment

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

This was inspired by the ubi backend. The aqua backend seems to always use extractVersion. Could you expand a bit on what your preferred solution might look like?

});
});

it('should set extractVersion if the version does not have leading v', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this already covered in the previous test?

if (isString(toolOptions.version_prefix)) {
const prefix = toolOptions.version_prefix;
if (prefix === '') {
// Empty prefix - no extractVersion needed if version has 'v'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Empty prefix - no extractVersion needed if version has 'v'
// Empty prefix - no extractVersion needed if version starts with 'v'

Comment on lines 125 to 140
if (isString(toolOptions.version_prefix)) {
const prefix = toolOptions.version_prefix;
if (prefix === '') {
// Empty prefix - no extractVersion needed if version has 'v'
if (!hasVPrefix) {
extractVersion = '^(?<version>.+)';
}
} else {
// Custom prefix - escape special regex chars
const escapedPrefix = prefix.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
extractVersion = `^${escapedPrefix}(?<version>.+)`;
}
} else if (!hasVPrefix) {
// Default: strip 'v' prefix if current version doesn't have it
extractVersion = '^v?(?<version>.+)';
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Less nested ifs

Suggested change
if (isString(toolOptions.version_prefix)) {
const prefix = toolOptions.version_prefix;
if (prefix === '') {
// Empty prefix - no extractVersion needed if version has 'v'
if (!hasVPrefix) {
extractVersion = '^(?<version>.+)';
}
} else {
// Custom prefix - escape special regex chars
const escapedPrefix = prefix.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
extractVersion = `^${escapedPrefix}(?<version>.+)`;
}
} else if (!hasVPrefix) {
// Default: strip 'v' prefix if current version doesn't have it
extractVersion = '^v?(?<version>.+)';
}
if (isNonEmptyString(prefix)) {
// Custom prefix - escape special regex chars
const escapedPrefix = prefix.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
extractVersion = `^${escapedPrefix}(?<version>.+)`;
}
// Empty prefix - no extractVersion needed if version has 'v'
if (isEmptyString(prefix) && !hasVPrefix) {
extractVersion = '^(?<version>.+)';
}
if (!hasVPrefix) {
// Default: strip 'v' prefix if current version doesn't have it
extractVersion = '^v?(?<version>.+)';
}

Copy link
Author

Choose a reason for hiding this comment

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

Although this seems a bit more performance intensive, I modified it. I did need to switch to else if to prevent the last if from overwriting extractVersion if it was set earlier. I could have an explicit check instead though if you prefer that.

Copy link
Collaborator

@RahulGautamSingh RahulGautamSingh left a comment

Choose a reason for hiding this comment

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

LGTM

@oschwald
Copy link
Author

Do you want me to rebase and force push? I see there is now a conflict.

@oschwald oschwald force-pushed the greg/github-backend branch from 196e80f to f91ed55 Compare January 27, 2026 16:36
@oschwald
Copy link
Author

I rebased this. Please let me know if there is anything else that needs to be changed.

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