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

Attach migration file to task bundle #1745

Merged
merged 5 commits into from
Dec 17, 2024

Conversation

tkdchen
Copy link
Contributor

@tkdchen tkdchen commented Dec 11, 2024

STONEBLD-3026

  • Introduce concepts of normal task (e.g. buildah) and extended task
    (e.g. buildah-20gb, buildah-min).
  • Refactor the code that processes task build and push.
  • For testing purpose, add a simple local cache in order to reduce the
    number of skopeo-inpsect runs for checking existence of task bundles.
    This could also be helpful to reduce the whole script execution during
    testing.
  • For testing purpose, script can be configured with environment
    variable TEST_TASKS to run and check the result for a small number of
    tasks.

To test the attach:

  • Open a new branch.
  • Bump version of a task in the YAML definition, e.g. task/buildah/0.2
  • Create task/buildah/0.2/migrations/.sh
  • Commit the changes
  • Run hack/build-and-push.sh by configuring it to push images to your
    own test image repository.
  • Find out the buildah task bundle.
  • Run `oras discover <bundle reference>'. The attached migration file
    should be listed.

@tkdchen tkdchen force-pushed the STONEBLD-3026-attach-migration-file branch 6 times, most recently from 474415f to 5fa4d8f Compare December 13, 2024 01:59
@tkdchen tkdchen marked this pull request as ready for review December 13, 2024 01:59
@tkdchen tkdchen requested a review from a team as a code owner December 13, 2024 01:59
@tkdchen
Copy link
Contributor Author

tkdchen commented Dec 13, 2024

/ok-to-test

@tkdchen tkdchen force-pushed the STONEBLD-3026-attach-migration-file branch 2 times, most recently from ce23ec2 to 046785d Compare December 13, 2024 07:57
STONEBLD-3026

* Introduce concepts of normal task (e.g. buildah) and extended task
  (e.g. buildah-20gb, buildah-min).
* Refactor the code that processes task build and push.
* For testing purpose, add a simple local cache in order to reduce the
  number of skopeo-inpsect runs for checking existence of task bundles.
  This could also be helpful to reduce the whole script execution during
  testing.
* For testing purpose, script can be configured with environment
  variable TEST_TASKS to run and check the result for a small number of
  tasks.

To test the attach:

* Open a new branch.
* Bump version of a task in the YAML definition, e.g. task/buildah/0.2
* Create task/buildah/0.2/migrations/<the new version>.sh
* Commit the changes
* Run hack/build-and-push.sh by configuring it to push images to your
  own test image repository.
* Find out the buildah task bundle.
* Run `oras discover <bundle reference>'. The attached migration file
  should be listed.
@tkdchen tkdchen force-pushed the STONEBLD-3026-attach-migration-file branch from 046785d to 3aba067 Compare December 13, 2024 08:14
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.

❤️ amazing commit message and "doc" in the script itself, thanks!

(will review soon, just wanted to highlight this first)

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 refactoring! Minor comments, mostly looks good

hack/build-and-push.sh Outdated Show resolved Hide resolved
hack/build-and-push.sh Outdated Show resolved Hide resolved
hack/build-and-push.sh Outdated Show resolved Hide resolved
hack/build-and-push.sh Outdated Show resolved Hide resolved
hack/build-and-push.sh Outdated Show resolved Hide resolved
hack/build-and-push.sh Show resolved Hide resolved
hack/build-and-push.sh Outdated Show resolved Hide resolved
Comment on lines +197 to +210
local -r bundle_ref="{
\"resolver\": \"bundles\",
\"params\": [
{\"name\": \"name\", \"value\": \"${task_name}\"},
{\"name\": \"bundle\", \"value\": \"${task_bundle_with_digest}\"},
{\"name\": \"kind\", \"value\": \"task\"}
]
}"
local -r task_selector="select(.name == \"${task_name}\" and .version == \"${task_version}\")"
find "$GENERATED_PIPELINES_DIR" "$CORE_SERVICES_PIPELINES_DIR" -maxdepth 1 -type f -name '*.yaml' | \
while read -r pipeline_file; do
yq e "(.spec.tasks[].taskRef | ${task_selector}) |= ${bundle_ref}" -i "${pipeline_file}"
yq e "(.spec.finally[].taskRef | ${task_selector}) |= ${bundle_ref}" -i "${pipeline_file}"
done
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice - I finally understand what this part of the code even does 🌮

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.

Create task/buildah/0.2/migrations/<the new version>.sh

Is this right? Should the migrations/ directory be under the version-specific directory, or should it be just task/buildah/migrations/?


While trying it out, I noticed we lost a lot of output:

QUAY_NAMESPACE=acmiel-test/tekton-catalog BUILD_TAG=$(date --iso-8601=date) SKIP_BUILD=1 SKIP_INSTALL=1 OUTPUT_TASK_BUNDLE_LIST=/tmp/task-bundle-list-konflux-ci OUTPUT_PIPELINE_BUNDLE_LIST=pipeline-bundle-list-konflux-ci hack/build-and-push.sh
*Warning*: This is an experimental command, it's usage and behavior can change in the next release(s)
*Warning*: This is an experimental command, it's usage and behavior can change in the next release(s)
*Warning*: This is an experimental command, it's usage and behavior can change in the next release(s)
*Warning*: This is an experimental command, it's usage and behavior can change in the next release(s)
✓ Pulled      0.2.2.sh                                                                                                                              71/71  B 100.00%  527µs
  └─ sha256:29f475854dcabc7419fd2d4b2da77b6c2f1fe98e78da8dab88f561b3fda90c1b
✓ Pulled      application/vnd.oci.image.manifest.v1+json                                                                                          783/783  B 100.00%     0s
  └─ sha256:8fcab734213322ad000455ebaf4384fb25af98523e5bc9643cdc5a20dddb2c56
*Warning*: This is an experimental command, it's usage and behavior can change in the next release(s)
*Warning*: This is an experimental command, it's usage and behavior can change in the next release(s)
*Warning*: This is an experimental command, it's usage and behavior can change in the next release(s)

The script no longers says what tasks it's pushing to quay.io

hack/build-and-push.sh Show resolved Hide resolved
hack/build-and-push.sh Outdated Show resolved Hide resolved
@tkdchen tkdchen requested a review from chmeliik December 16, 2024 09:47
@tkdchen
Copy link
Contributor Author

tkdchen commented Dec 16, 2024

@chmeliik Comments are addressed. PTAL.

@chmeliik
Copy link
Contributor

Nice, output is much more readable now. Just not sure about this part:

Create task/buildah/0.2/migrations/.sh

Is this right? Should the migrations/ directory be under the version-specific directory, or should it be just task/buildah/migrations/?

@chmeliik
Copy link
Contributor

Nice, output is much more readable now. Just not sure about this part:

Create task/buildah/0.2/migrations/.sh

Is this right? Should the migrations/ directory be under the version-specific directory, or should it be just task/buildah/migrations/?

(it would make sense to me under the version-specific directory as well, just checking if that's the intention)

@tkdchen
Copy link
Contributor Author

tkdchen commented Dec 16, 2024

Is this right? Should the migrations/ directory be under the version-specific directory, or should it be just task/buildah/migrations/?

@chmeliik Sorry. I forgot to reply to this topic. Yes, the former is intended. That means a version-specific task has its own migrations and not mixed with other versions'.

@tkdchen tkdchen force-pushed the STONEBLD-3026-attach-migration-file branch from 6277368 to c071ad9 Compare December 16, 2024 12:37
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.

LGTM 👍

@tkdchen tkdchen added this pull request to the merge queue Dec 17, 2024
Merged via the queue into konflux-ci:main with commit 7d7e52e Dec 17, 2024
16 checks passed
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.

2 participants