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: add git ref graphql field #6376

Merged
merged 3 commits into from
Feb 2, 2024
Merged

feat: add git ref graphql field #6376

merged 3 commits into from
Feb 2, 2024

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Jan 9, 2024

Fix #6375.

As suggested by @sagikazarmark, it feels odd to put refs/pull/5/merge and similar into a commit/branch/tag selection - it's a ref, so we should have a generic ref selection.

To handle the rust codegen case, we need to do some magic since ref is a strict keyword. We can resolve this using a raw identifier. @dagger/sdk-rust I've hacked something in to get the codegen to work, but let me know if you want to take a different approach.


In general, it's a little bit strange that we have so many functions that all have exactly the same underlying implementation - e.g. you can put a commit ID in a branch and it works fine. Maybe we should consider prefixing it with refs/heads to branch, and prefixing with refs/tags to tag - we could then require that the args to commit be a SHA ID.

Then ref would be the only "generic" field that passes directly to buildkit.

Copy link
Contributor

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@gerhard
Copy link
Member

gerhard commented Jan 31, 2024

It's unfortunate that this has been sitting open for as long as it did, and now there are a bunch of conflicts to deal with.

How do you want to proceed @jedevc?

@jedevc
Copy link
Member Author

jedevc commented Jan 31, 2024

Rebased, and also pulled in my fix from buildkit for moby/buildkit@6a8d2ca (since we have a copy-paste job, I'd forgotten about that).

As a follow-up, I'll work on removing our custom git source if I can.

Signed-off-by: Justin Chadwell <me@jedevc.com>
This is a cherry-pick of moby/buildkit@6a8d2ca

Signed-off-by: Justin Chadwell <me@jedevc.com>
Copy link
Contributor

@vito vito left a comment

Choose a reason for hiding this comment

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

Nice seeing this, it always bugged me to use this API in scenarios where you're not exactly sure whether the given value is a commit, branch name, or tag (and don't need to know)

Comment on lines +441 to +457
var (
partialRef = "refs/" + strings.TrimPrefix(ref, "refs/")
headRef = "refs/heads/" + strings.TrimPrefix(ref, "refs/heads/")
tagRef = "refs/tags/" + strings.TrimPrefix(ref, "refs/tags/")
annotatedTagRef = tagRef + "^{}"
)
var sha, headSha, tagSha string
for _, line := range lines {
lineSha, lineRef, _ := strings.Cut(line, "\t")
switch lineRef {
case headRef:
headSha = lineSha
case tagRef, annotatedTagRef:
tagSha = lineSha
case partialRef:
sha = lineSha
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use a quick comment explaining what this does

Looks like it's looking for matches by checking for potential expanded paths for the given ref in the output?

Copy link
Member Author

@jedevc jedevc Feb 2, 2024

Choose a reason for hiding this comment

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

Ah this is pulled directly from moby/buildkit@6a8d2ca, and was part of my logic for starting on #6560.

Ideally I'd like to just not need to duplicate any of this code from buildkit, but sadly I think this is still required until I can submit some upstream changes.

Also added a comment for this though.

Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc merged commit c8b7f77 into dagger:main Feb 2, 2024
43 checks passed
@jedevc jedevc deleted the git-add-ref branch February 2, 2024 11:47
@kjuulh
Copy link
Contributor

kjuulh commented Feb 10, 2024

@jedevc Sorry for not getting back to you, my github notifications are still messed up. The r# is fine. It is common for rust. It isn't the prettiest, but it should be fine for now.

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.

✨ Allow pulling refs from a git repo
4 participants