feat: add withPlainSecretVariable

[TomChv]

Resolves: #5000

[helderco]

Good work, only minor/trivial changes. 👍

Not entirely sure about the name, but can't think of a better one. 🙂

[TomChv]

@helderco PR update and ready for review :)

[vito]

Not a fan of the name here either, since it sounds like you're doing something wrong ("secrets in plaintext"). Technically the SetSecret API also works by passing a plaintext value, so I don't think this API should feel any scarier.

Personally I don't see much value in having an API shorthand here in the first place, since there can be value in decoupling secrets from all the places they're used, especially if a secret is used in multiple places.

The status quo is slightly repetitive if you keep env var names + secret names matching all the time, but doesn't seem that awful:

container.WithSecretVariable("FOO", client.SetSecret("FOO", "hey"))
container.WithPlainSecretVariable("FOO", "hey")

If we can find a good name I'd be onboard though. It's possible I just don't have the empathy for above yet, and I don't mind sugar in the API for really common use cases, especially if it makes it easier to quickly script a bunch of things.

Some ideas:

• WithSecretVariableValue
• WithSecretVariableInline

I don't love any of'em but I think it's important to drop "Plain."

[helderco]

Not a fan of the name here either, since it sounds like you're doing something wrong ("secrets in plaintext").

Yes, that’s exactly why!

I also considered WithSecretVariableValue but to me it reads more like ”Secret value” instead of “a string value that will later be a Secret”. Then I considered WithSecretVariableFromPlain but that’s long and the FromXXX is sort of redundant with the argument.

Personally I don't see much value in having an API shorthand here in the first place

I was kind of wondering the same thing. It reminds me of #4835 but that was a harder problem because it couldn’t easily be solved in the API.

However, this is easy to abstract away. There’s a docs example in Python where an Env enum is used to reduce duplication/verbosity and type safety when loading environment variables from host and using either as plaintext or secrets. You can see the usage is pretty lean:

addr = await ctr.with_registry_auth(  
    "docker.io",  
    Env.DOCKERHUB_USERNAME,  
    Env.DOCKERHUB_PASSWORD.as_secret(dagger_client),
)

And the abstraction is easy to do:

class Env(StrEnum):  
    ...     
    DOCKERHUB_USERNAME = auto()  
    DOCKERHUB_PASSWORD = auto()  
  
    def as_secret(self, client: dagger.Client) -> dagger.Secret:  
        return client.set_secret(self.name.lower(), self.value)

Another example is to make a chainable util for this, e.g., for reducing the duplication of mentioning the same host env var name:

func SecretHostEnv(c *dagger.Client, name string) dagger.ContainerWithFunc {
    return func(ctr *dagger.Container) *dagger.Container {
        return ctr.WithSecretVariable(
            name,
            client.SetSecret(name, os.GetEnv(name))
        )
    }
}

ctr := c.Container().
    From("alpine").
    ...
    WithEnvVariable("ABI_USER")
    With(SecretHostEnv(c, "ABI_PASS")).
    WithExec(...)

Note that this isn’t strictly equal to what the PR does but that’s the point. These are conveniences for specific use cases and depend on users’ needs. While we want to make it easy for the most common use cases, we also need to consider how much there’s to gain.

My point is the same as @vito’s:

• I’m concerned that plain in the name sounds like it’s unsafe, that’s why I made the clarification suggestion to the field’s description.
• Current status quo isn’t awful. I don’t mind the sugar but don’t see much value in it.
• There’s a good point in decoupling secrets.

I also wonder about the future for secrets. I know this has been discussed but I don’t have full context. My understanding is that we want a pluggable system for multiple different sources for secrets. I’m not sure if the current setSecret is meant for all of those (e.g., 1Password, Vault…) or just the default in-memory store. It makes more sense to me that it’s the latter since you could need multiple secret sources in the same pipeline. If that’s true, than that makes it even more compelling to decouple setSecret (how you get it) from Secret (where you use it).

[gerhard]

@TomChv would you mind trying out changie new for this PR? I am very curious to find out how it works in practice for you. FWIW: #5408 (comment)

[TomChv]

@TomChv would you mind trying out changie new for this PR? I am very curious to find out how it works in practice for you. FWIW: #5408 (comment)

I think this PR will be one hold until I've fix #3457, but I can try too add a release note for #5488

[TomChv]

PR Closed, see #5000 (comment)