#API design question for `Git` 🧵

1 messages · Page 1 of 1 (latest)

nimble lantern
#

Noticed that we have entirely different ways of configuring auth for ssh vs pat. For ssh, we just pass top level args to dag.Git, but for pat/header auth we have a dag.Git.withAuthToken

#

Ideally we shouldn't be doing it like this, it should be consistent. We can easily fix this though, and make it consistent (and deprecate whichever we don't choose).

However! Not quite sure which one to go for. I'm really ambivalent honestly, though maybe args make the most amount of sense?

#

cc @steel kelp @hexed vector @olive quest

#

I remember discussing this ages back at some point (like a year +), but since I'm back in reworking + improving the git api (in the context of https://github.com/dagger/dagger/pull/9730), figured we should fix this one too

steel kelp
#

I remember being partial to the separate methods for auth instead of constructor.

nimble lantern
#

mm, it is a bit neater to chain that way.
i think the only argument i have against it is that it maybe makes sense to group a private url right next to a token for it? instead of splitting them up into two separate calls.
but yeah, i don't particularly think it matters that much. i can't see why you'd want to change it more than once, so it would always appear after dag.Git anyways.

nimble lantern
#

okay one reason I'm now leaning towards the constructor: there are now two ways to make a GitRepository - through Query.git and Directory.asGit

#

imo it doesn't make a huge amount of sense to be able to configure auth on a git repo that's come from the local directory

#

or imagine a function taking a GitRepostory - that object should be ready to use, it shouldn't be possible to pass in a semi created state, where the auth isn't correctly setup yet

#

fyi @marble quartz, we were talking about this recently

olive quest
#

Catching up...

nimble lantern
livid shadow
#

Hey @nimble lantern , I just now seeing this thread. Thank you for the tag. I think it makes sense to have the auth in the constructor at the top level. That can propagate to all downstream calls. It will be a breaking change for me but that's not a big deal right now.

nimble lantern
#

(fyi, we'd just deprecate for now, we won't remove it immediately)

livid shadow
nimble lantern
#

that's how it works now - i'd like to preserve that property

livid shadow
#

Sorry, when I say "remote ops", I mean stuff like git fetch --tags or git pull they need auth

nimble lantern
#

but how are you getting it to do that? you're mounting it as a directory?

livid shadow
#

yes

nimble lantern
livid shadow
nimble lantern
#

i'm reasonably sure we won't be able to do this sadly, directories don't have associated secrets, only containers do

#

ideally, i'd like to solve this by just having the Git API itself able to do this itself

#

so you don't need to fetch additional things - you could import remotes, and do it through there

#

unless, we had Container.WithGitRef directly

#

then we could do pass through

#

not entirely sure, but yes, agreed these problems are kind of related.

#

even thinking about https://github.com/dagger/dagger/issues/10200 for "credential inheritance", we can't just always inherit credentials from the parent.
it's very easy to imagine a case where you have a public remote and a private remote (i've worked places with this setup), and you could very reasonably need different sets of creds for this.

olive quest
#

@nimble lantern I don't understand what the "two options" were at the beginning of this thread. You mentioned PAT support and Git.withAuthToken, is that actually part of the API today?

nimble lantern
olive quest
#

But I don't understand how it can work, since Git() creates the GitRepository, what does the withAuthToken apply to?

nimble lantern
#

the problem is that there is a disparity between how you pass ssh auth and http auth params.
http auth is passed only via with
ssh auth is passed only via the constructor

nimble lantern
#

and the operations it does

olive quest
#

I see, it relies on dag.Git being lazy

#

But this is distinct from native PAT support where dag.Git() on its own loads the full git configuration from the host and applies it implicitly right?

#

I was just confused since you pass the URL to dag.Git() but the auth credentials for that URL afterwards. Opposite pattern from dag.Container()

#

I guess consistency has never been our thing (to my eternal frustration)

nimble lantern
olive quest
#

By the way Container.withRegistryAuth also doesn't make sense for distinct reasons

nimble lantern
#

actually. would make much more sense to have From and Publish take credentials

#

and deprecate withRegistryAuth in the same vein imo

olive quest
#

Yes. That was the original spec. At some point liberties were taken in the implementation

#

Sometimes I wish I didn't care about consistency, I would be less stressed, and less annoying to others

nimble lantern
#

sure, but i get that the api isn't perfect, but i'm trying to get things better now, i have some cycles to do this

#

i can just leave it alone, but that's not really helpful to anyone

olive quest
#

I think constructor as you suggested is good

#

I just wanted to make sure you didn't take inspiration from Container.withRegistryAuth in your quest for consistency

nimble lantern
olive quest
nimble lantern
#

neat okay! it's on the list now

livid shadow
#

sorry, busy day. I'll catch up and get back to you with an example of what I'm trying to do. It's possible that I'm doing something that's not recommended.

livid shadow
#

Caught up. I didn't quite follow the latter part of the discussion. But this is my requirement.

  • It's a vanilla CI module that's contextual to the repo.
  • After build, test, publish, I want to tag the git repo.
  • I need auth. Dagger itself already has auth for my repo (because it cloned it) but I can't use it.
  • Today I'm authenticating like this - https://github.com/dagger/dagger/issues/9858

I'm not opposed to keep doing that. Lmk if I'm making things difficult for myself 🙂

nimble lantern
livid shadow