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

Pushcontainer image ref #338

Merged
merged 2 commits into from
Apr 22, 2021
Merged

Pushcontainer image ref #338

merged 2 commits into from
Apr 22, 2021

Conversation

samalba
Copy link
Contributor

@samalba samalba commented Apr 16, 2021

This is a proposal that fixes #303

The change allows certain ops to export meta data to the image fs, under /dagger/. We could reuse the same mechanism for fetch-git to write things like /dagger/commit or /dagger/remote, etc...

I was initially not ok to alter the pushed image, but after testing it, I think it's fine as the pushed image does not contain the meta-data. It's only available within the #up pipeline.

Also, I like the fact that it does not break the current op.#Export api which I like (IMHO).

I've also updated the jamstack example to try it in a real use case:

	ref: {
                string

		#up: [
			// Build the docker image
			op.#DockerBuild & {
				context: source
			},
			// Push the image to the registry
			op.#PushContainer & {
				ref: pushTarget
			},
			op.#Export & {
				source: "/dagger/image_ref"
				format: "string"
			},
		]
	}

Signed-off-by: Sam Alba <sam.alba@gmail.com>
Signed-off-by: Sam Alba <sam.alba@gmail.com>
@shykes
Copy link
Contributor

shykes commented Apr 20, 2021

	ref: pushTarget
		#up: [
			// Build the docker image
			op.#DockerBuild & {
				context: source
			},
			// Push the image to the registry
			op.#PushContainer & {
				ref: pushTarget
			},
			op.#Export & {
				source: "/dagger/image_ref"
				format: "string"
			},
		]
	}

I think there’s a typo in your example? I’m assuming the first two lines should actually be:

ref: {
    string
    // ...
}

Is that right?

@shykes
Copy link
Contributor

shykes commented Apr 20, 2021

@aluzzardi wdyt?

@samalba
Copy link
Contributor Author

samalba commented Apr 20, 2021

ref: {
    string
    // ...
}

Is that right?

Yes correct, I made a typo. The example in the PR is correct. I updated my comment.

@aluzzardi
Copy link
Member

Overall, I think this fixes the immediate issue. However I think it's a bit limiting, due to how #Export works.

The drawbacks are:

  • You must attach #PushContainer to a string scalar if you're planning to export (e.g. can't export to a particular struct field)
  • You can't export more than one #PushContainer field: if you want to export both image_ref and image_digest, you need two pipelines
  • You can't export more than one operation at a time: e.g. if you #FetchGit then #PushContainer on the same pipeline, you can't export both /dagger/git_ref and /dagger/image_digest

But you can workaround all those problems by using small pipelines and #Load'ing between pipelines, so this works.

@samalba
Copy link
Contributor Author

samalba commented Apr 21, 2021

Overall, I think this fixes the immediate issue. However I think it's a bit limiting, due to how #Export works.

The drawbacks are:

* You must attach `#PushContainer` to a string scalar if you're planning to export (e.g. can't export to a particular struct field)

* You can't export more than one `#PushContainer` field: if you want to export both `image_ref` and `image_digest`, you need two pipelines

* You can't export more than one operation at a time: e.g. if you `#FetchGit` then `#PushContainer` on the same pipeline, you can't export both `/dagger/git_ref` and `/dagger/image_digest`

But you can workaround all those problems by using small pipelines and #Load'ing between pipelines, so this works.

I think we can remove all those drawbacks by supporting a field target that will allow multiple exports.

Example:

	image: {
                ref: string
                digest: string

		#up: [
			// Build the docker image
			op.#DockerBuild & {
				context: source
			},
			// Push the image to the registry
			op.#PushContainer & {
				ref: pushTarget
			},
			op.#Export & {
				source: "/dagger/image_ref"
				format: "string"
                                target: "ref"
			},
			op.#Export & {
				source: "/dagger/image_digest"
				format: "string"
                                target: "digest"
			},
		]
	}

And this can be added later without breaking the existing code. I would use this.

@aluzzardi
Copy link
Member

I think we can remove all those drawbacks by supporting a field target that will allow multiple exports.

Yep. Make sense.

Also, food for thoughts: If we're adding a @dagger(computed) annotation for inputs, perhaps we could make use of it to replace #Export?

e.g.

	image: {
                ref: string @dagger(computed, source="/dagger/image_ref", format="string")
                digest: string @dagger(computed, source="/dagger/image_digest", format="string")

		#up: [
			// Build the docker image
			op.#DockerBuild & {
				context: source
			},
			// Push the image to the registry
			op.#PushContainer & {
				ref: pushTarget
			},
		]
	}

@samalba
Copy link
Contributor Author

samalba commented Apr 21, 2021

image: {
ref: string @dagger(computed, source="/dagger/image_ref", format="string")
digest: string @dagger(computed, source="/dagger/image_digest", format="string")

Nice, I probably prefer this. Also format= can be optional as "string" is a good default.

@samalba
Copy link
Contributor Author

samalba commented Apr 21, 2021

Since @aluzzardi proposal with the annotation would not change the use of /dagger/* for op's metadata (which is the main purpose of this PR). I'd suggest merging this PR, then dealing with the migration from op.#Export to annotations in a separate PR.

@shykes
Copy link
Contributor

shykes commented Apr 21, 2021

I’m OK with merging it but just a heads up I would like to take @aluzzardi ‘s proposal even further, which will break configurations based on this PR. If you would rather not break configurations post merge, then we should wait a little longer before merging...

@shykes
Copy link
Contributor

shykes commented Apr 21, 2021

Question @samalba: in your example, is image.ref always the same as \(pushTarget)@\(image.digest) ? I’m wondering if we could get away with only exporting one value instead of two.

@samalba
Copy link
Contributor Author

samalba commented Apr 21, 2021

Question @samalba: in your example, is image.ref always the same as \(pushTarget)@\(image.digest) ? I’m wondering if we could get away with only exporting one value instead of two.

Yes, for this example, just exported the digest is supposed to give the same full-ref. I usually prefer to take the full-ref from buildkit instead, in case the URL the full image url would change during the push (not supposed to).

@shykes
Copy link
Contributor

shykes commented Apr 21, 2021

Here’s what I think we can do once @dagger(computed) is merged. Basically: when executed in the context of a computed value, some operations export values. What is exported is documented for each operation. For example

  • PushContainer exports the pushed ref and digest
  • Exec exports the contents of /dagger/export.json or /dagger/export.txt. It is an error for both files to exist.
  • PushGit exports the pushed git ref.
  • etc.

Example:

source: dagger.#Artifact                                                   
pushTarget: string                                                         

image: {                                                                   
        ref: string 
        digest: string 
        
        @dagger(computed)
        #up: [                                                             
                // Build the docker image                                  
                op.#DockerBuild & {                                        
                        context: source
                },
                // Push the image to the registry                          
                // When executed in the context of a computed value, exports pushed ref and digest. See docs.
                op.#PushContainer & {                                      
                        ref: pushTarget
                },
        ]               
}

cmd: {
    foo: int
    @dagger(computed)
    #up: [
        dagger.#Load & { from: image },
        dagger.#Exec & { args: [“sh”, “-c”, “jq -n .foo=42 > /dagger/export.json”] },
    ]
}

Benefits:

  • Less things to type to get a computed value to work
  • One simple rule to remember: “when in a computed value, some operations export stuff”
  • No need to go through the filesystem for operations other than Exec. PushContainer, PushGit can just export straight to cue.
  • Less duplication between shell script and cue. Don’t need to choose a path, write it once in the shell script, once in the cue config etc.

@samalba
Copy link
Contributor Author

samalba commented Apr 22, 2021

I agree with the benefits, but I see some downsides:

  • the annotation not being attached to a key makes it less readable (IMO)
  • most shell scripts will need json for exporting data, jq is the only way to manipulate json properly and is not trivial to use

@shykes
Copy link
Contributor

shykes commented Apr 22, 2021

  • the annotation not being attached to a key makes it less readable (IMO)

I didn’t understand this one sorry.

  • most shell scripts will need json for exporting data, jq is the only way to manipulate json properly and is not trivial to use

I think once we have some usage data, we can add sugar, for example:

Example 1: export json-typed scalar without jq. This actually would work out of the box.

foo: {
  int @dagger(computed)
  #up: [
    dagger.#Load & { ... },
    dagger.#Exec & { args: [“sh”, “-c”, “echo 42 > /dagger/export.json”]},
  ]
}

Example 2: optionally specify an export key in exec.

cmd: {
  @dagger(computed)
  foo: int
  hello: string
  #up: [
    dagger.#Load & { ... },
    // Export ‘foo’ without jq
    dagger.#Exec & {
        args: [“sh”, “-c”, “echo 42 > /dagger/export.json”]
        exportTarget: “foo”
    },
    // Export ‘hello’ without jq
    dagger.#Exec & {
        args: [“sh”, “-c”, “echo world > /dagger/export.txt”]
        exportTarget: “hello”
    },
  ]
}

@samalba samalba merged commit d000b29 into main Apr 22, 2021
@samalba samalba deleted the pushcontainer-image-ref branch April 22, 2021 18:49
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.

Export the ref of a pushed image using "push-container"
3 participants