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

Replace hardcoded github.com/gitlab.com checks with detectPlatform() #28102

Closed
rarkins opened this issue Mar 24, 2024 · 1 comment · Fixed by #28945
Closed

Replace hardcoded github.com/gitlab.com checks with detectPlatform() #28102

rarkins opened this issue Mar 24, 2024 · 1 comment · Fixed by #28945
Labels
priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:refactor Refactoring or improving of existing code

Comments

@rarkins
Copy link
Collaborator

rarkins commented Mar 24, 2024

Describe the proposed change(s).

We have some inconsistency in our code where sometimes we check if a host is e.g. github.com or includes the string "github" in its hostname, and other times we have a more sophisticated check using detectPlatform(). We should do a code review for remaining hardcoded checks and see which can be replaced with detectPlatform().

@rarkins rarkins added priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:refactor Refactoring or improving of existing code labels Mar 24, 2024
@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Apr 26, 2024

I could only find this one instance where we can directly use detectPlatform

protected override hasValidToken(config: BranchUpgradeConfig): {

Apart from this I found the following 4 instances where it seems we can use the detectPlatform function but cannot/should not :

  1. export function parseArchiveUrl(
  2. export function parseUrlPath(
  3. function getDepName(url: string | null): string | null {

(In the above 3 cases, we should not use detectPlatform because parseUrl is already called within the fns and the other fields returned from the parseUrl fn are also used in them. If we use detectPlatform just for the url matching then we will be repeating similar logic as detectPlatform calls parseUrl too.)


  1. if (hostname === 'github.com' || detectPlatform(repository) === 'github') {
    logger.debug({ repository, hostname }, 'Found github dependency');
    return { datasource: GithubTagsDatasource.id };
    }
    if (hostname === 'gitlab.com') {
    logger.debug({ repository, hostname }, 'Found gitlab dependency');
    return { datasource: GitlabTagsDatasource.id };

(We cannot as its only the hostname which is being compared and not the full url )

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:refactor Refactoring or improving of existing code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants