Skip to content

feat(RELEASE-1989): python for publish-index-images tasks#2241

Open
midnightercz wants to merge 1 commit into
developmentfrom
RELEASE-1989
Open

feat(RELEASE-1989): python for publish-index-images tasks#2241
midnightercz wants to merge 1 commit into
developmentfrom
RELEASE-1989

Conversation

@midnightercz

Copy link
Copy Markdown
Contributor

tasks/internal/publish-index-image and
tasks/managed/publish-index-image

internal bash scripts are replaced with python scripts from release-service-utils

Describe your changes

Relevant Jira

Checklist before requesting a review

  • I have marked as draft or added do not merge label if there's a dependency PR
    • If you want reviews on your draft PR, you can add reviewers or add the release-service-maintainers handle if you are unsure who to tag
  • My commit message includes Signed-off-by: My name <email>
  • I read CONTRIBUTING.MD and commit formatting
  • I have run the README.md generator script in .github/scripts/readme_generator.sh and verified the results using .github/scripts/check_readme.sh
  • If an AI agent was used, I marked that via a commit footer like Assisted-By: Cursor

@qodo-code-review

qodo-code-review Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit de5bbe3)

Warning

/review is deprecated. Use /agentic_review instead (removal date not yet scheduled).

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 Security concerns

Supply chain integrity:
The switch from a digest-pinned image to a mutable tag (quay.io/jluza/release-service-utils:RELEASE-1989) combined with imagePullPolicy: Always increases the risk of pulling unexpected image contents over time. Prefer pinning to an immutable digest for reproducible and safer execution.

⚡ Recommended focus areas for review

Supply Chain

The task image was switched from a digest-pinned reference to a mutable tag with imagePullPolicy: Always, which can lead to non-reproducible runs and increases the risk of unintentionally pulling a different image than expected. Consider pinning to an immutable digest (even if built from the branch) or otherwise ensuring provenance and reproducibility.

image: >-
  quay.io/jluza/release-service-utils:RELEASE-1989
imagePullPolicy: Always
computeResources:
Test Robustness

Temporary file creation and here-doc redirection should be more defensive: quote variables in redirections/exports, and ensure the temp file is cleaned up (trap) to avoid leaking files across test runs. Also verify that the python3() shim argument forwarding (${@:3}) matches how the task invokes python consistently.

export _python3=$(which python3)
fake_setup=$(mktemp)
cat <<'EOF' > $fake_setup
---
inspect:
  - match:
      image: "docker://quay.io/match-target-digest"
      format: "{{.Digest}}"  # optional
    return: "sha256:match1234567890"  # string when format specified
  - match:
      image: "docker://quay.io/target"
      format: "{{.Digest}}"  # optional
    return: "sha256:target1234567890"  # string when format specified
  - match:
      image: "docker://registry-proxy.engineering.redhat.com/foo"
      format: "{{.Digest}}"  # optional
    return: "sha256:0987654321fedcba"  # string when format specified
  - match:
      image: "docker://registry-proxy.engineering.redhat.com/foo"
      format: "{{.Digest}}"  # optional
    return: "sha256:0987654321fedcba"  # string when format specified
  - match:
      image: "docker://registry-proxy.engineering.redhat.com/fail"
    return:
      error: "skopeo inspect failed"  # string when format specified
      returncode: 1
copy:
  - match:
      source: "docker://registry-proxy.engineering.redhat.com/match@sha256:match1234567890"
      destination: "docker://quay.io/match-target-digest"
    # omit return for success
  - match:
      source: "docker://registry-proxy.engineering.redhat.com/foo@sha256:0987654321fedcba"
      destination: "docker://quay.io/target"
    # omit return for success
  - match:
      source: "docker://quay.io/source@sha256:abcdef1234567890"
      destination: "docker://quay.io/target"
    # omit return for success
  - match:
      source: "docker://registry-proxy.engineering.redhat.com/fail@sha256:0987654321fedcba"
      destination: "docker://quay.io/target"
    return:
      success: false
      error: "skopeo copy failed"  # string when format specified
      returncode: 1
EOF
export RELEASE_SERVICE_UTILS_FAKE_SKOPEO_SETUP=$fake_setup

function python3() {
  "$_python3" -c "
from fake import patch_skopeo_client;
patch_skopeo_client();
from publish_index_image import main;
main();" "${@:3}"
}
Input Validation

The value read from .fbc.publishingCredentials is passed directly into the python module; if it is missing/null/empty, the step may fail later with a less actionable error. Consider validating it early (e.g., jq -e / explicit non-empty check) and failing with a clear message.

credentials="$(jq -r '.fbc.publishingCredentials' "$DATA_FILE")"

python3 -m managed_publish_index_image \
  --request-timeout "$(params.requestTimeout)" \
  --publishing-credentials "${credentials}" \
  --retries "$(params.retries)" \
  --task-git-url "$(params.taskGitUrl)" \
  --task-git-revision "$(params.taskGitRevision)" \
  --pipeline-run-id "$(params.pipelineRunUid)" \
  --ir-results-file "$(params.dataDir)/$(params.internalRequestResultsFile)"
📄 References
  1. No matching references available

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Warning

/improve is deprecated. Use /agentic_review instead (removal date not yet scheduled).

Inline suggestions were posted as code suggestions.

Comment thread tasks/managed/publish-index-image/publish-index-image.yaml
Comment thread tasks/internal/publish-index-image-task/publish-index-image-task.yaml Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the managed and internal “publish index image” Tekton tasks to replace embedded bash logic with Python entrypoints provided by the release-service-utils image.

Changes:

  • Swap bash implementations for python3 -m managed_publish_index_image and python3 -m publish_index_image.
  • Update the task step image reference for both tasks (currently to quay.io/jluza/release-service-utils:RELEASE-1989).
  • Simplify the managed task script by removing the inline internalrequest creation loop.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
tasks/managed/publish-index-image/publish-index-image.yaml Replaces the managed publish loop with a Python module invocation and updates the step image reference.
tasks/internal/publish-index-image-task/publish-index-image-task.yaml Replaces skopeo-based bash publishing logic with a Python module invocation and updates the step image reference.
Comments suppressed due to low confidence (1)

tasks/internal/publish-index-image-task/publish-index-image-task.yaml:85

  • The task defines a requestMessage result, and the internal publish-index-image pipeline surfaces it, but the new Python invocation no longer writes anything to $(results.requestMessage.path). This will cause task/pipeline consumers and the existing task tests to fail. Please ensure the Python implementation writes the intended message to the Tekton result file (or plumb the result path into the module and write it there).
          --target-index "$(params.targetIndex)" \
          --retries "$(params.retries)" \
          --source-credential-path /mnt/publishingCredentials/sourceIndexCredential \
          --target-credential-path /mnt/publishingCredentials/targetIndexCredential


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tasks/managed/publish-index-image/publish-index-image.yaml
Comment thread tasks/managed/publish-index-image/publish-index-image.yaml
@midnightercz

Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@midnightercz

Copy link
Copy Markdown
Contributor Author

/retest

@red-hat-konflux

Copy link
Copy Markdown
Contributor

All PipelineRuns for this commit have already succeeded. Use /retest <pipeline-name> to re-run a specific pipeline or /test to re-run all pipelines.

@midnightercz

Copy link
Copy Markdown
Contributor Author

/ok-to-test

@red-hat-konflux

Copy link
Copy Markdown
Contributor

All PipelineRuns for this commit have already succeeded. Use /retest <pipeline-name> to re-run a specific pipeline or /test to re-run all pipelines.

@midnightercz

Copy link
Copy Markdown
Contributor Author

/ok-to-test

@midnightercz

Copy link
Copy Markdown
Contributor Author

/ok-to-test

2 similar comments
@midnightercz

Copy link
Copy Markdown
Contributor Author

/ok-to-test

@midnightercz

Copy link
Copy Markdown
Contributor Author

/ok-to-test

@midnightercz

Copy link
Copy Markdown
Contributor Author

/retest

@qodo-app-for-konflux-ci

Copy link
Copy Markdown

PR Code Suggestions ✨

Warning

/improve is deprecated. Use /agentic_review instead (removal date not yet scheduled).

Inline suggestions were posted as code suggestions.

Comment thread tasks/internal/publish-index-image-task/tests/mocks.sh
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Warning

/improve is deprecated. Use /agentic_review instead (removal date not yet scheduled).

Inline suggestions were posted as code suggestions.

Comment thread tasks/internal/publish-index-image-task/tests/mocks.sh
@qodo-app-for-konflux-ci

Copy link
Copy Markdown

PR Code Suggestions ✨

Warning

/improve is deprecated. Use /agentic_review instead (removal date not yet scheduled).

Inline suggestions were posted as code suggestions.

Comment thread tasks/managed/publish-index-image/publish-index-image.yaml
Comment thread tasks/internal/publish-index-image-task/tests/mocks.sh
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Warning

/improve is deprecated. Use /agentic_review instead (removal date not yet scheduled).

Inline suggestions were posted as code suggestions.

Comment thread tasks/managed/publish-index-image/publish-index-image.yaml
Comment thread tasks/internal/publish-index-image-task/tests/mocks.sh
@qodo-app-for-konflux-ci

Copy link
Copy Markdown

PR Code Suggestions ✨

Warning

/improve is deprecated. Use /agentic_review instead (removal date not yet scheduled).

Inline suggestions were posted as code suggestions.

Comment thread tasks/managed/publish-index-image/publish-index-image.yaml
@midnightercz midnightercz force-pushed the RELEASE-1989 branch 2 times, most recently from fb16139 to de5bbe3 Compare June 1, 2026 07:26
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Warning

/improve is deprecated. Use /agentic_review instead (removal date not yet scheduled).

Inline suggestions were posted as code suggestions.

Comment thread tasks/managed/publish-index-image/publish-index-image.yaml
Comment thread tasks/internal/publish-index-image-task/tests/mocks.sh
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Warning

/improve is deprecated. Use /agentic_review instead (removal date not yet scheduled).

Inline suggestions were posted as code suggestions.

Comment thread tasks/managed/publish-index-image/publish-index-image.yaml
Comment thread tasks/internal/publish-index-image-task/tests/mocks.sh
@qodo-app-for-konflux-ci

Copy link
Copy Markdown

PR Code Suggestions ✨

Warning

/improve is deprecated. Use /agentic_review instead (removal date not yet scheduled).

Inline suggestions were posted as code suggestions.

Comment thread tasks/internal/publish-index-image-task/tests/mocks.sh
Comment thread tasks/managed/publish-index-image/publish-index-image.yaml
tasks/internal/publish-index-image and
tasks/managed/publish-index-image

internal bash scripts are replaced with python scripts
from release-service-utils

Signed-off-by: Jindrich Luza <jluza@redhat.com>
Assisted-By: gemini
Co-authored-by: qodo-code-review[bot]
  <151058649+qodo-code-review[bot]@users.noreply.github.com>
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Warning

/improve is deprecated. Use /agentic_review instead (removal date not yet scheduled).

Inline suggestions were posted as code suggestions.

Comment thread tasks/internal/publish-index-image-task/tests/mocks.sh
@qodo-app-for-konflux-ci

Copy link
Copy Markdown

PR Code Suggestions ✨

Warning

/improve is deprecated. Use /agentic_review instead (removal date not yet scheduled).

Inline suggestions were posted as code suggestions.

@qodo-app-for-konflux-ci

Copy link
Copy Markdown

PR Code Suggestions ✨

Warning

/improve is deprecated. Use /agentic_review instead (removal date not yet scheduled).

Inline suggestions were posted as code suggestions.

Comment thread tasks/internal/publish-index-image-task/tests/mocks.sh
--source-credential-path /mnt/publishingCredentials/sourceIndexCredential \
--target-credential-path /mnt/publishingCredentials/targetIndexCredential | \
awk '{printf "%s", (NR==1 ? "" : ORS) $0}' | \
tee "$(results.requestMessage.path)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This works, but I wonder if it would be cleaner to have the Python script write requestMessage directly (e.g. RESULT_REQUEST_MESSAGE env + tekton.result_paths_from_env, same as check_fbc_opt_in)? Then you would not need the stdout pipe/awk layer, and log output cannot accidentally end up in the result.

@happybhati happybhati left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor comment, overall looks good, waiting on Utils change to be merged and some conflicts on this PR.

--retries "$(params.retries)" \
--source-credential-path /mnt/publishingCredentials/sourceIndexCredential \
--target-credential-path /mnt/publishingCredentials/targetIndexCredential | \
awk '{printf "%s", (NR==1 ? "" : ORS) $0}' | \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You might use instead the result_paths_from_env in the python script instead of falling back to shell:
https://github.com/konflux-ci/release-service-utils/blob/main/scripts/python/helpers/tekton.py#L131

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants