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: push Dockerfile to registry #1129

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

tkdchen
Copy link
Contributor

@tkdchen tkdchen commented Jul 5, 2024

STONEBLD-2522

Add a new task push-dockerfile to push Dockerfile to registry as an OCI
artifact.

Task is configurable to accept varaints of Dockerfile with different
names, like Containerfile.

Use `oras pull' to get the image.

This new task is added to docker-build pipeline and enabled by default.

Trusted Artifacts version of push-dockerfile is created accordingly and
updated into the docker-build-oci-ta pipeline.

@tkdchen tkdchen force-pushed the STONEBLD-2522-push-Dockerfile branch from cae09d3 to dea4b84 Compare July 5, 2024 05:20
@tkdchen tkdchen marked this pull request as ready for review July 5, 2024 05:38
@tkdchen tkdchen force-pushed the STONEBLD-2522-push-Dockerfile branch 4 times, most recently from 883d38b to 1859ba7 Compare July 5, 2024 07:58
task/build-image-manifest/0.1/build-image-manifest.yaml Outdated Show resolved Hide resolved
task/buildah-oci-ta/0.1/buildah-oci-ta.yaml Outdated Show resolved Hide resolved
@tkdchen tkdchen force-pushed the STONEBLD-2522-push-Dockerfile branch 5 times, most recently from bca31e5 to 9968d21 Compare July 6, 2024 02:19
@tkdchen tkdchen requested a review from mmorhun July 6, 2024 03:51
@tkdchen tkdchen requested review from tisutisu, rcerven and mkosiarc July 8, 2024 11:30
Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

I'll also note that it will not be possible for the release service (or any other consumer) to verify that the content of the .dockerfile image is still the same as what the build pipeline pushed. Which is probably fine, IIUC the Dockerfile image is probably just a nice-to-have informational kind of thing

task/build-image-manifest/0.1/build-image-manifest.yaml Outdated Show resolved Hide resolved
task/build-image-manifest/0.1/build-image-manifest.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

Did you consider making a separate task for pushing the Dockerfile to the registry? It doesn't really make any sense to me have it as part of the build tasks

@tkdchen tkdchen requested a review from chmeliik July 9, 2024 06:12
@tkdchen
Copy link
Contributor Author

tkdchen commented Jul 9, 2024

I haven't seen any necessity to make a separate task. As an associated artifacts images like .sbom and .att, it is natural to push it as a step of the build.

@chmeliik
Copy link
Contributor

chmeliik commented Jul 9, 2024

I haven't seen any necessity to make a separate task. As an associated artifacts images like .sbom and .att, it is natural to push it as a step of the build.

.att and .sig are pushed by Tekton Chains, not by the build task. I would argue .sbom shouldn't be pushed by the build task either. In the current situation, we have ~9 build tasks that all have to copy-paste the same logic because they have to fulfill this undefined "interface" for what a build task should do. When some of those things don't actually make sense.

Our tasks don't really make sense as upstream parts of the Tekton ecosystem. Now we're injecting this functionality into every build task because that's more convenient for the Red Hat container catalog.

On the other hand, there is no good mechanism for sharing artifacts between tasks - other than Trusted Artifacts, which involves pushing to the registry in some form anyway.

@tkdchen tkdchen force-pushed the STONEBLD-2522-push-Dockerfile branch 4 times, most recently from f5d5e79 to 2e8a3ba Compare July 11, 2024 12:44
@tkdchen tkdchen requested review from mkosiarc and chmeliik July 11, 2024 12:46
@tkdchen tkdchen force-pushed the STONEBLD-2522-push-Dockerfile branch from 2e8a3ba to 169ad6a Compare July 11, 2024 12:54
@tkdchen
Copy link
Contributor Author

tkdchen commented Jul 11, 2024

PTAL. The previous changes are reworked by adding a new Tekton task push-build-file. Once review is done, I'll squash all the commits before merge.

Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

Nice, this looks better to me 👍

.gitignore Show resolved Hide resolved
annotations:
tekton.dev/pipelines.minVersion: "0.12.1"
tekton.dev/tags: "image-build, appstudio"
name: push-build-file-oci-ta
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add the recipe.yaml (every oci-ta task should have one) for this task, so that auto-generation works properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

task/build-image-manifest/0.1/build-image-manifest.yaml Outdated Show resolved Hide resolved
@chmeliik
Copy link
Contributor

FYI, I think the failing ec-tasks-check should be resolved by rebasing on main

@tkdchen tkdchen force-pushed the STONEBLD-2522-push-Dockerfile branch 2 times, most recently from 471879c to fef74dc Compare July 12, 2024 09:05
@tkdchen
Copy link
Contributor Author

tkdchen commented Jul 12, 2024

Rebased. Added recipel.yaml and add a task result IMAGE_URL.

@tkdchen tkdchen requested a review from chmeliik July 12, 2024 10:03
@tkdchen tkdchen force-pushed the STONEBLD-2522-push-Dockerfile branch from 12d6689 to 74faefc Compare July 15, 2024 04:35
@tkdchen
Copy link
Contributor Author

tkdchen commented Jul 15, 2024

/retest

@tkdchen tkdchen force-pushed the STONEBLD-2522-push-Dockerfile branch from 74faefc to 2b12446 Compare July 15, 2024 05:31
@tkdchen
Copy link
Contributor Author

tkdchen commented Jul 15, 2024

The task push-build-file is renamed to push-dockerfile. PTAL.

@tkdchen tkdchen requested a review from chmeliik July 15, 2024 05:49
STONEBLD-2522

Add a new task push-dockerfile to push Dockerfile to registry as an OCI
artifact.

Task is configurable to accept varaints of Dockerfile with different
names, like Containerfile.

Use `oras pull' to get the image.

This new task is added to docker-build pipeline and enabled by default.

Trusted Artifacts version of push-dockerfile is created accordingly and
updated into the docker-build-oci-ta pipeline.

Signed-off-by: Chenxiong Qi <[email protected]>
@tkdchen tkdchen force-pushed the STONEBLD-2522-push-Dockerfile branch from 6c761c5 to ca7141d Compare July 15, 2024 11:26
@tkdchen
Copy link
Contributor Author

tkdchen commented Jul 15, 2024

Squashed all commits and rebased.

@tkdchen tkdchen added this pull request to the merge queue Jul 15, 2024
Merged via the queue into konflux-ci:main with commit cc54e9b Jul 15, 2024
7 checks passed
@tkdchen tkdchen deleted the STONEBLD-2522-push-Dockerfile branch July 16, 2024 02:04
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.

4 participants