@aluzzardi There's still a FIXME about writing to stdout when using the tty logger, which comes from pre-europa:

How should this be properly fixed?

Thanks a lot @helderco, this is great!

I'll do a proper review tomorrow, I have a couple of suggestions on how to handle output printing

Hey @helderco, I have an overall architecture suggestion regarding this feature. This is up for debate, I can be convinced otherwise:

• Make output: values: [name=string] a task, just like output: directories (e.g. plan/task/outputvalue.go, similar to plan/task/outputdirectory.go)
• Add OutputValues to plancontext so that tasks (specifically outputValueTask) can add values to the output (e.g. similar to plancontext.LocalDirs, as defined in plancontext/localdir.go
• have cmd/up.go print those values from plancontext.OutputValues. Either OutputValues returns a List() of values (like LocalDirs), or it could have a Print(w io.Writer) function.
  • Either way, nothing outside of cmd/ should be using viper -- that's strictly for CLI handling

Doing it this make makes it so that:

• output: values are not a special case in the code base (they'd work just like any other task and output)
• It decouples the Plan schema from the implementation (like it currently is)
  • Easier to make changes to one (e.g. how we collect outputs) and the other (e.g. how we display them)
• Leaves the door open for other tasks to add outputs (e.g. if we wanted to, in the future output: files and directories could add an entry here to specify where they wrote the information)
• It allows us to rely on the task model which is pretty solid
  • For instance, if !p.Source().LookupPath(path).Exists() won't be necessary anymore -- if no output: values is there, then the code won't get triggered

Happy to hear your thoughts about this, either here or live on Discord.

Hey @aluzzardi, I'm totally on board with all of this.

Thanks for the tip about plancontext, I like that. I just rather not have tasks write to values automatically because to me, the best about this is to have full control (as a user) over what gets output. See #566 about intermediary outputs, which I've always found annoying. So I'd rather reference something I need from outputs: files in outputs: values.

About viper, that makes total sense, I wasn't entirely sure where to put that code, just that I wanted to keep it separate.

I wanted to make this a task initially, but as far as I'm aware, tasks are matched by the $dagger: task: _name path which requires that I add a contents field:

dagger.#Plan & {
    outputs: values: {
        value1: contents: "value1"
        value1: contents: "value2"
    }
}

Instead of

dagger.#Plan & {
    outputs: values: {
        value1: "value1"
        value1: "value2"
    }
}

That's the whole reason I implemented it like this. If you look at how #1351 was originally, it had a contents, but @shykes suggested (hidden comment):

We definitely need something like this. Thoughts on DX:

• [...]

• This would be a sort mirror of inputs: params which is a simple map of key to value (no intermediary contents). So we should keep them consistent. This means either:

  • Option 1: harmonize with inputs: simple map everywhere. For example: outputs: values: “build image”: actions.build.push.result
  • Option 2: harmonize with outputs: extra contents field everywhere. For example: inputs: params: gitCommit: contents: string

Any preference between option 1 and 2?

Still, I do think the task approach is much better. I can migrate to a task anyway, adding the contents convention, and we can track a possible simplification down the line, that benefits not only this task but others.

The downside is changing config for users if we do remove contents.

WDYT? 🙂

Thanks for the tip about plancontext, I like that. I just rather not have tasks write to values automatically because to me, the best about this is to have full control (as a user) over what gets output. See #566 about intermediary outputs, which I've always found annoying. So I'd rather reference something I need from outputs: files in outputs: values.

Yep, makes sense to me!

About viper, that makes total sense, I wasn't entirely sure where to put that code, just that I wanted to keep it separate.

Yes indeed. There's two options, either moving that code into cmd or just taking explicit string/bool/etc arguments in the plan package and doing the viper calls in cmd. This is what we already do in up.go, e.g.

	p, err := plan.Load(ctx, plan.Config{
		Args:   args,
		With:   viper.GetStringSlice("with"),
		Target: viper.GetString("target"),
	})

Here for instance plan.Load doesn't use viper -- instead it takes explicit arguments and we do the viper wiring in cmd/up.go.

I wanted to make this a task initially, but as far as I'm aware, tasks are matched by the $dagger: task: _name path which requires that I add a contents field:
That's the whole reason I implemented it like this. If you look at how #1351 was originally, it had a contents, but @shykes suggested (hidden comment):
Still, I do think the task approach is much better. I can migrate to a task anyway, adding the contents convention, and we can track a possible simplification down the line, that benefits not only this task but others.
WDYT? 🙂

You're right, I missed that! Indeed it's not possible with the current DX.

@shykes Thoughts on this?

For context, it's about the 2 options you cited (mentioned in #1486 (comment)). The TL;DR: is: if we were to use contents field (Option 2) it would make the architecture of this feature much nicer (mirror of output: directories). No contents field (Option 1) does work but it's a special code path.

I wanted to make this a task initially, but as far as I'm aware, tasks are matched by the $dagger: task: _name path which requires that I add a contents field:
That's the whole reason I implemented it like this. If you look at how #1351 was originally, it had a contents, but @shykes suggested (hidden comment):
Still, I do think the task approach is much better. I can migrate to a task anyway, adding the contents convention, and we can track a possible simplification down the line, that benefits not only this task but others.
WDYT? 🙂

You're right, I missed that! Indeed it's not possible with the current DX.

@shykes Thoughts on this?

For context, it's about the 2 options you cited (mentioned in #1486 (comment)). The TL;DR: is: if we were to use contents field (Option 2) it would make the architecture of this feature much nicer (mirror of output: directories). No contents field (Option 1) does work but it's a special code path.

I’m fine with it. A few notes though:

• Assume this will not change. After Europa, we’re going to try very hard not to break user configurations, because it’s very painful for everyone. So removing the contents field later is not on the table IMO. Before proceeding, make sure this doesn’t change your opinion.
• If we had the ability to match tasks by path in addition to by content, I believe this change wouldn’t be needed 😇
• Maybe we will find a use for additional fields beyond contents?

• Maybe we will find a use for additional fields beyond contents?

That's what I keep thinking.

• If we had the ability to match tasks by path in addition to by content, I believe this change wouldn’t be needed 😇

I think this can be done without relying on path, but it needs to be done with a definition instead:

_#outputValue: {
    #dagger: _task: name: "OutputValue"
    _
}

outputs: values: [string]: _#outputValue

outputs: values: foo: "bar"

https://cuelang.org/play/?id=HnfLaFo7Hs3#cue@export@cue

Pros:

• Best of both worlds:
  • Better DX where it makes sense;
  • Better implementation architecture (task);

Cons:

• Less consistency (Europa: The Great Compromise of December 16, 2021. (spec). #1234)
• What if you want to add more fields later?

• Assume this will not change. After Europa, we’re going to try very hard not to break user configurations, because it’s very painful for everyone. So removing the contents field later is not on the table IMO. Before proceeding, make sure this doesn’t change your opinion.

I'm good with this.

• If we had the ability to match tasks by path in addition to by content, I believe this change wouldn’t be needed 😇

You're right -- that's definitely an option we could consider instead.

It's quite some work but we could go ahead with this PR as is (without contents field, not using the task model) and refactor away once we have more advanced task matching (no user visible changes).

• Assume this will not change. After Europa, we’re going to try very hard not to break user configurations, because it’s very painful for everyone. So removing the contents field later is not on the table IMO. Before proceeding, make sure this doesn’t change your opinion.
• Maybe we will find a use for additional fields beyond contents?

Yeah, It boils down to this -- is having contents useful for potential extra fields, or is it bothersome to type for no good reason. I don't have a strong opinion either way. @shykes @helderco ?

I think this can be done without relying on path, but it needs to be done with a definition instead:

Indeed, but since it requires changing $dagger to #dagger, it's a pretty big change. We would need to change all tasks to #dagger for this to work. The alternative is custom task matching just for output values, but at that point, we could just do path based as @shykes suggested.

• If we had the ability to match tasks by path in addition to by content, I believe this change wouldn’t be needed 😇

You're right -- that's definitely an option we could consider instead.

It's quite some work but we could go ahead with this PR as is (without contents field, not using the task model) and refactor away once we have more advanced task matching (no user visible changes).

• Assume this will not change. After Europa, we’re going to try very hard not to break user configurations, because it’s very painful for everyone. So removing the contents field later is not on the table IMO. Before proceeding, make sure this doesn’t change your opinion.
• Maybe we will find a use for additional fields beyond contents?

Yeah, It boils down to this -- is having contents useful for potential extra fields, or is it bothersome to type for no good reason. I don't have a strong opinion either way. @shykes @helderco ?

Argh, this is a difficult decision. Not so much whether to change outputs.values but whether to change inputs.params. Since that’s where the downside of the extra contents fields will be most pronounced, and if you change one, you have to change the other for consistency.

• For example dagger up —with ‘inputs.params.foo: “bar”’ becomes dagger up —with ‘inputs.params.foo.contents: “bar”’
• And actions: foo: invertFluxCapacitor: inputs.params.invertFluxCapacitor becomes actions: foo: invertFluxCapacitor: invertFluxCapacitor.contents

I can think of potentially exciting ways to use extra fields - for example to specify how to render a web component for a given parameter or value :) But is it worth the extra verbosity? I can’t tell right now.

Can someone try this in a real or realistic configuration? @gerhard @jlongtine @talentedmrjones in changelog.com for example? How does it feel it we make this change?

Argh, this is a difficult decision. Not so much whether to change outputs.values but whether to change inputs.params. Since that’s where the downside of the extra contents fields will be most pronounced, and if you change one, you have to change the other for consistency.

I don't think the consistency principle necessarily applies quite in this way. Consistency says that I look for the contents field rather than a different field for each thing, but so far that field exists on most if not everything dagger gives me. I don't expect to have to provide the contents field for things I give to dagger; if that were the case we'd have inputs:directories:[string]:contents, inputs:secrets:[string]:contents and so on but that's clearly too verbose and unnecessary.

Argh, this is a difficult decision. Not so much whether to change outputs.values but whether to change inputs.params. Since that’s where the downside of the extra contents fields will be most pronounced, and if you change one, you have to change the other for consistency.

I don't think the consistency principle necessarily applies quite in this way. Consistency says that I look for the contents field rather than a different field for each thing, but so far that field exists on most if not everything dagger gives me. I don't expect to have to provide the contents field for things I give to dagger; if that were the case we'd have inputs:directories:[string]:contents, inputs:secrets:[string]:contents and so on but that's clearly too verbose and unnecessary.

Those fields do exist, though. You do have to reference inputs.directories.foo.contents and inputs.secrets.foo.contents. Same for output directories and output secrets.

Input Params/ output values are different from secrets and directories, because the contents consumed and produced in the DAG are directly exposed to the user. Because of that difference, we can justify skipping the contents field: but then we have to skip it on both ends, otherwise input params and output values would be inconsistent with each other.

✔️ Deploy Preview for devel-docs-dagger-io ready!

🔨 Explore the source changes: d3e9c10

🔍 Inspect the deploy log: https://app.netlify.com/sites/devel-docs-dagger-io/deploys/6213d43fb455160008bacffc

😎 Browse the preview: https://deploy-preview-1486--devel-docs-dagger-io.netlify.app

Ping on this one

We have found ways to support either approach (with or without contents) using the task model.

It's a matter of picking the right DX -- I've no opinion on the matter, anyone with a strong opinion either way, @shykes @helderco @talentedmrjones?

Can someone try this in a real or realistic configuration? @gerhard @jlongtine @talentedmrjones in changelog.com for example? How does it feel it we make this change?

This is outside of my top 3 items currently, but I've taken a mental note to circle back if it's still relevant by the time my cache clears.

Picking this up. As a reminder, we must choose between two options:

Option 1: harmonize with inputs: simple map everywhere. For example: outputs: values: “build image”: actions.build.push.result
Option 2: harmonize with outputs: extra contents field everywhere. For example: inputs: params: gitCommit: contents: string

At this point I think we should go with option 1.

• If we had the ability to match tasks by path in addition to by content, I believe this change wouldn’t be needed 😇

You're right -- that's definitely an option we could consider instead.

It's quite some work but we could go ahead with this PR as is (without contents field, not using the task model) and refactor away once we have more advanced task matching (no user visible changes).

This would be my preference. Benefits:

• We can merge this quickly without @helderco having to rework it
• Consistent with inputs without having to change their schema
• Implementation can be improved later without impacting users
• Implementation improvement will unblock other things too (for example path matching makes it safe to remove hidden fields)
• In theory we could find a use for more fields beyond contents, but we can't think of any, so it's too risky a gamble

SGTM, let's go ahead with this PR then

I meant to reply, but I've been really busy as you can imagine 🙂

@shykes summed up my thoughts fully.

There's still the issue with the logger that needs to be fixed.

@aluzzardi, do you have an ideia how it should be fixed? Do you want to merge this and track the logging issue in another PR? I can look into it some more on monday.

Do you want to merge this and track the logging issue in another PR? I can look into it some more on Monday.

I think that it would be best to leave this one for Monday, no rush.

I am keen to test it in the context of https://github.com/dagger/dagger/blob/main/pkg/universe.dagger.io/examples/changelog.com/main.cue, as @shykes was suggesting.

Looking forward to it 👍🏻

@helderco Either we print the table using the logger (I believe we can use the logger as a io.Writer), or we try to do the same thing as pre-Europa (stop the tty logger before printing), although I think it may be buggy.

Yes, I already had this working yesterday. I'm just logging to Info(), which works well in all cases as far as I'm aware.

I'll add a doc page later.

@aluzzardi @shykes, as I said, it's easier to just log to info instead of writing directly to stdout. It works fine for my use case, but it breaks the use case of piping into jq.

So this doesn't work:

$ dagger up --output-format json | jq '.build'

But you can still save into a file:

$ dagger up --output-format json --output state.json
$ jq '.build' state.json

To support piping outputs:

1. We need to output to stdout, in which case we do have to stop the TTY logger before printing;
2. If dagger isn't being run with an allocated TTY we still have to resort to logging to info instead, but if that's your environment you can just save to a file and use that (bringing us to square one).

Is the convenience of piping worth the added complexity?

I defer to @aluzzardi on this one. No strong opinion.

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.

I came across this via GitHub notifications.

What are your latest thoughts on this @helderco?

With Client API (#1668), we have yet to discuss the future of this feature. I asked about this and @shykes replied:

Where would you keep inputs: params and outputs: values?

Good question. They are out of scope from client, but there are hints of possible solutions:

• human-readable outputs: Action API #1598 defines a $dagger: after: string field to control what is printed to the user

• machine-readable inputs & outputs: 0.2 CLI #1558 defines a new UI where the focus is on a single action. Since actions have fields already.. maybe those can act as params/values and there is no need to duplicate? More design & discussion needed.

Originally posted in #1597 (comment)

Also discussed in the counterpart params.

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.

This no longer applies with dagger do.

• For inputs, @jlongtine is working on dagger do: expose input fields as command-line flags #1719
• For outputs, we should simply print the action's fields. This can be further refined, but we can start fresh on this topic in another issue.

Thank you @helderco for carrying this alongside #1668 , even though it didn't get merged, it helped us get to a solution.