Skip to content

Gitlab support#171

Open
bit-man wants to merge 11 commits into
bpkg:masterfrom
bit-man:feature/gitlab-support
Open

Gitlab support#171
bit-man wants to merge 11 commits into
bpkg:masterfrom
bit-man:feature/gitlab-support

Conversation

@bit-man

@bit-man bit-man commented Jun 3, 2024

Copy link
Copy Markdown

Currently bpkg repositories can be stored using Github. This PR adds Gitlab support
Can be tested by running bpkg install bit-man/forrest-tools located at https://gitlab.com/bit-man/forrest-tools

Comment thread lib/install/install.sh Outdated
return 0
}

bpkg_solve_uri() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am curious how we can support other remotes in the future for URLs that need to be resolved like this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe bpkg_solve_uri could look for a user space function that can format a pathspec like this...

thinking out loud:

# in ~/.bpkgrc

bpkg_resolve_uri_pathspec () {
  # ...
}

in the bpkg_solve_uri function we could do:

bpkg_solve_uri() {
  if test -f bpkg_resolve_uri_pathspec; then
    bpkg_resolve_uri_pathspec $@
    return $?
  fi

  # ... rest of implementation
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Maybe using some configuration instead of code. Let's say that in $HOME/.bpkg/urlmap file with one regex that on matching gives the URI to be used. @jwerle Maybe your proposal fits better in bpkg. Just let me know

https://gitlab*:$user/$name/-/raw/$version
https://github*:$user/$name/$version

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I started looking into adding this feature a while back. Essentially providing configurable repository sites. The default would be GitHub, but we could add additional. The bpkg.json could have a section for repositories to provide project specific dependencies.

On top of that, bpkg.sh provides a collection JSON and that would be a default, but others could setup their own (ie: internal teams and projects). Collections (as they are now) could be updated to get the latest project repositories. Subsequent project/system installs would use this list first, unless a dependency is specified with a FQDN.

I'm going to look into this PR further and provide additional feedback.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's OK to use ShellSpec for testing? Following #133

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added external URL resolve tested with ShellSpec. Test execution : shellspec --shell /bin/bash

@jwerle jwerle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

left some comments

@jwerle jwerle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice work! This looks good to me at a glance! I'd need to test it still

@hyperupcall hyperupcall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a great feature! I just had two questions

Comment thread lib/utils/url.sh Outdated
}

bpkg_resolve_uri() {
[[ $(type -t bpkg_resolve_uri_pathspec) == function ]] && \

@hyperupcall hyperupcall Jun 8, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: I think something like this is more readable and removes the shellcheck warning:

if [[ $(type -t bpkg_resolve_uri_pathspec) == function ]]; then
	bpkg_resolve_uri_pathspec "$@" || bpkg_resolve_uri_default "$@"
else
	bpkg_resolve_uri_default "$@"
fi

It preserves the behavior where if bpkg_resolve_uri_pathspec fails, we then use our bpkg_resolve_uri_default function to resolve the uri (i think it's easy to miss that detail). But, should this be considered a bug? If I define a bpkg_resolve_uri_pathspec myself, I expect it to be the only function used to resolve URI, even if my function returns with a non-exit code. It might be better to keep the "string" blank as-is, or to print an error instead.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

the idea behind bpkg_resolve_uri_pathspec its not to be the only solver but a complimentary one to help everyone else solve their problem and stil let the heavy lifting to bpkg

About the code suggestion is a very good one. I take it

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed at be225d1

@hyperupcall hyperupcall Jun 21, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the update; I do still think that both bpkg_resolve_uri_pathspec and bpkg_resolve_uri_default shouldn't be invoked in the same codepath sequentially like that. I would be interested in hearing the other maintainers weigh in, but in any case, I'm not trying to stop this code from being merged as-is - it's a smaller detail.

Comment thread lib/utils/url.sh

@hyperupcall hyperupcall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a great feature! I just had two questions, posted above. (this comment was posted in an attempt to remove my "requested changes" status, but that didn't work)

@jwerle jwerle requested review from hyperupcall and jwerle June 18, 2024 13:16
@bit-man

bit-man commented Nov 16, 2024

Copy link
Copy Markdown
Author

Given that this PR has not been merged yet, I'm maintaining a fork that its only addition is Gitlab support for bpkg. Sync and maintenance will cease when this PR is merged

@jwerle

jwerle commented Feb 24, 2025

Copy link
Copy Markdown
Member

i'll give this another review - cc @hyperupcall

Comment thread lib/install/install.sh Outdated
@Potherca

Potherca commented Aug 17, 2025

Copy link
Copy Markdown
Member

The code looks good (IMHO), is there anything other than the Shellcheck violations that need to be resolved for this to be merged?

@jwerle

jwerle commented Oct 13, 2025

Copy link
Copy Markdown
Member

cc @bit-man any way we can get the shell check stuff addressed?

@bit-man

bit-man commented Nov 21, 2025

Copy link
Copy Markdown
Author

The logs for this run have expired and are no longer available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants