fix(CLOUDDST-30777): log improvements for deploy-fbc-operator

[JAVGan]

This PR contains the following changes:

Validate parameters for deploy-fbc-operator

This commit introduces a new task named validate-parameters which will be executed first in the pipeline.

This newly added task will raise errors when required secrets/parameters are not set as well as emit warnings for the ones which are missing so the end user can properly debug.

The goal of this change is to prevent a scenario which the pipeline gets stuck without any error messages due to missing secrets, as well as early failing/warning for missing fields.

Log skipped tasks for deploy-fbc-operator

This commit enhances the logging for deploy-fbc-operator by informing which tasks are going to be skipped for non "push" events, such as "pull_request" or "retest-all-comment".

JIRA

Refers to CLOUDDST-30777

[JAVGan]

@nmars can you please check whether this makes sense?

[JAVGan]

@nmars @yashvardhannanavati I also updated the 1.0 for the sake of "why not" and it's raising errors on Validate Tasks and Pipelines YAMLs but it's unrelated to my change AFIAK

[nmars]

Also tagging @asergienk for her opinion.

[asergienk]

Could you please update the README file to add new tasks: https://github.com/JAVGan/tekton-integration-catalog/blob/5a7cc1e805d7b4af3a1542e1d7e214e6062d90f9/pipelines/deploy-fbc-operator/0.2/README.MD

[qodo-app-for-konflux-ci]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📜 Skill insights (0)

1. Validation task unbound vars ✓ Resolved 🐞 Bug ≡ Correctness

Description

validate-parameters reads ${SNAPSHOT}, ${PACKAGE_NAME}, etc. under set -euo pipefail but
does not define task params or step env bindings, so the script will fail with an unbound-variable
error before emitting the intended validation output. This can block the entire pipeline at the very
first task.

Code

pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[R98-129]

+              #!/usr/bin/env bash
+              set -euo pipefail
+
+              if [[ -z "${SNAPSHOT}" ]]; then
+                echo "SNAPSHOT parameter is required"
+                exit 1
+              fi
+
+              if [[ -z "${PACKAGE_NAME}" ]]; then
+                echo "PACKAGE_NAME parameter is empty: the step will discover the package name in the FBC fragment."
+              fi
+
+              if [[ -z "${CHANNEL_NAME}" ]]; then
+                echo "CHANNEL_NAME parameter is empty. The step will determine the default channel name of the selected package."
+              fi
+
+              if [[ -z "${CREDENTIALS_SECRET_NAME}" ]]; then
+                echo "CREDENTIALS_SECRET_NAME parameter is required"
+                exit 1
+              fi
+
+              if [[ -z "${OCI_REF}" ]]; then
+                echo "OCI_REF parameter is required"
+                exit 1
+              fi
+
+              if [[ -z "${REPO_TOKEN}" &&  -z "${REPO_KEY}" ]]; then
+                echo "WARNING: REPO_TOKEN and REPO_KEY parameters are not set."
+              elif [[ -z "${REPO_TOKEN}" || -z "${REPO_KEY}" ]]; then
+                echo "REPO_TOKEN and REPO_KEY should be set together."
+                exit 1
+              fi

Relevance

●●● Strong

Team fixes pipeline-breaking bash issues under set -euo pipefail (accepted in PR #279); similar
deploy-fbc-operator pipeline bugs fixed (PR #215).

PR-#279
PR-#215

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new validate-parameters task uses multiple variables as bash environment variables while `set
-u` is enabled, but it does not bind the corresponding Tekton pipeline parameters into the step
environment, meaning those $VAR references will be unset and trigger an immediate abort. Other
inline steps in the same pipeline demonstrate the expected repository pattern by explicitly mapping
$(params.X) into env: before referencing $X in bash, highlighting that this task is missing
that required wiring.

pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[92-137]
pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[138-152]
pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[635-662]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[94-139]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[140-154]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[647-674]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The inline `validate-parameters` task uses bash variables like `$SNAPSHOT`/`$OCI_REF` while running with `set -u`. Since Tekton pipeline params are not automatically available as shell environment variables, the step will error with an "unbound variable" and never reach the intended `echo`/`exit 1` validation messages.

## Issue Context
Other inline steps in this same pipeline explicitly map `$(params.X)` into `env:` before using `$X` in bash scripts, which is the expected pattern in this repo.

## Fix Focus Areas
- pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[92-152]
- pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[94-154]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Non-push runs fail early ✗ Dismissed 🐞 Bug ≡ Correctness

Description

validate-parameters unconditionally exits when CREDENTIALS_SECRET_NAME or OCI_REF is empty,
even though the tasks that consume those values only run for test-event-type == push. This will
cause pull_request/retest PipelineRuns to fail at the first task instead of cleanly skipping
provisioning/deploy tasks.

Code

pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[R115-123]

+              if [[ -z "${params.CREDENTIALS_SECRET_NAME}" ]]; then
+                echo "CREDENTIALS_SECRET_NAME parameter is required"
+                exit 1
+              fi
+
+              if [[ -z "${params.OCI_REF}" ]]; then
+                echo "OCI_REF parameter is required"
+                exit 1
+              fi

Relevance

●● Moderate

No prior precedent on conditional required params by event; only optional-param gating seen in
PR276/277.

PR-#276
PR-#277

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new validation task exits if CREDENTIALS_SECRET_NAME/OCI_REF are empty, but
deploy-operator (which mounts the credentials secret and uses OCI_REF to push artifacts) is
explicitly gated to run only on push events, so these values are not required for non-push flows.

pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[92-138]
pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[561-570]
pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[601-606]
pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[972-987]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[94-140]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[573-582]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`validate-parameters` currently enforces `CREDENTIALS_SECRET_NAME` and `OCI_REF` as required for *all* PipelineRuns, but these are only needed on the push-only execution path. As a result, non-push events (e.g. `pull_request`, `retest-all-comment`) will fail immediately, defeating the purpose of adding `log-non-push-tasks`.

## Issue Context
Downstream tasks that mount/use these parameters are gated with `when: test-event-type in ["push", "Push"]`.

## Fix Focus Areas
- pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[92-139]
- pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[561-606]
- pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[972-987]
- pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[94-141]
- pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[573-619]
- pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[975-991]

## What to change
1. Move the validation that depends on push-only params (e.g., `CREDENTIALS_SECRET_NAME`, `OCI_REF`, and any other deploy-only inputs) to a task that runs **after** `parse-metadata`.
2. Gate those checks with a `when` clause identical to the push-only tasks (i.e., only validate them when `$(tasks.parse-metadata.results.test-event-type)` is `push`).
3. Keep only universally-required checks (e.g., `SNAPSHOT`) in the initial validation task if you still want a first-task fast-fail.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Slack link depends on parse-metadata 🐞 Bug ☼ Reliability

Description

parse-metadata now runs after validate-parameters, so any early validation failure prevents
parse-metadata from producing application-name; the finally Slack notification still
interpolates $(tasks.parse-metadata.results.application-name), making the log URL unusable exactly
when validation is supposed to improve debuggability.

Code

pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[R130-131]

+      runAfter:
+        - validate-parameters

Relevance

●● Moderate

PR #277 added Slack log link using parse-metadata result; no precedent addressing missing results on
early validation failure.

PR-#277
PR-#276

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR adds a hard dependency so parse-metadata cannot run if validate-parameters fails, while
the Slack finally task still builds its Konflux UI link using parse-metadata’s application-name
result.

pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[92-133]
pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[109-117]
pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[1013-1040]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[94-145]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[111-119]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[1015-1042]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`parse-metadata` is now gated behind `validate-parameters`, but the `finally.slack-webhook-notification` message still depends on `$(tasks.parse-metadata.results.application-name)`. If validation fails early, that result is not produced, so the Slack message cannot construct a valid Konflux UI URL.

## Issue Context
This PR intentionally adds early-failing validation to avoid silent/stuck pipelines. Early failure paths should still keep failure notifications and log links functional.

## Fix Focus Areas
- pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[129-133]
- pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[1013-1042]
- pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[131-135]
- pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[1016-1042]

## What to change
Implement one of the following (preferred options first):
1. **Make the Slack link independent of `parse-metadata`** (recommended): remove `/applications/$(tasks.parse-metadata.results.application-name)` from the URL and use a URL form that only relies on `$(context.pipelineRun.namespace)` and `$(context.pipelineRun.name)`.
2. **Provide an explicit pipeline param for application name** (e.g., `APPLICATION_NAME`) and use that in the Slack message, so it exists even if `parse-metadata` never runs.
3. If the link must remain based on `parse-metadata`, then **ensure `parse-metadata` runs before validation** (but that likely conflicts with the goal of validating first).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Pipeline README filename casing 📘 Rule violation ⚙ Maintainability ⭐ New

Description

The pipeline directory uses README.MD instead of the required case-sensitive README.md, so it
does not satisfy the README presence requirement and may break tooling that expects README.md
exactly.

Code

pipelines/deploy-fbc-operator/0.1/README.MD[R7-11]

+1. **Validate Parameters (`validate-parameters`)**
+
+   Validates whether all required parameters are properly passed to the pipeline.
+
+2. **Parse Metadata (`parse-metadata`)**  

Relevance

● Weak

Repo repeatedly keeps README.MD in pipeline dirs (e.g., deploy-fbc-operator docs updated without
renaming) indicating non-enforcement.

PR-#228
PR-#277
PR-#276

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2653 requires pipeline directories to include a file literally named README.md.
In this PR, the documentation files present in the affected pipeline directories are named
README.MD, which means there is no correctly cased README.md filename to satisfy the
requirement.

Rule 2653: Document each Pipeline and PipelineRun directory with a README.md
pipelines/deploy-fbc-operator/0.1/README.MD[1-11]
pipelines/deploy-fbc-operator/0.2/README.MD[1-27]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Compliance requires a `README.md` file (case-sensitive) in each Pipeline/PipelineRun documentation directory. The pipeline documentation files are currently named `README.MD`, which counts as missing `README.md` for compliance and can break tooling expecting the exact filename.

## Issue Context
The files were modified in this PR, so the naming issue is in-scope; update the documentation filename casing so the directory contains `README.md` exactly.

## Fix Focus Areas
- pipelines/deploy-fbc-operator/0.1/README.MD[1-11]
- pipelines/deploy-fbc-operator/0.2/README.MD[1-27]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Inconsistent task name formatting ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The new README task list entry for log-non-push-tasks omits the parenthesized task-name style used
by the other entries (e.g., Parse Metadata (parse-metadata)), reducing consistency/scanability
and making task-name copy/paste patterns uneven. The same inconsistency appears in both the 0.1 and
0.2 pipeline READMEs.

Code

pipelines/deploy-fbc-operator/0.1/README.MD[R16-18]

+3. **Log Non Push Tasks `log-non-push-tasks`**
+   Informs which tasks are going to be skipped for non `push` events, such as
+   `pull_request` or `retest-all-comment`

Relevance

●● Moderate

No prior accepted/rejected reviews found enforcing README task-list “Title (task)” formatting;
only unrelated README feedback exists.

PR-#180
PR-#228
PR-#276

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Both READMEs use the **Title (task-name)** convention for other tasks, but the newly added
log-non-push-tasks entry deviates from that convention.

pipelines/deploy-fbc-operator/0.1/README.MD[7-20]
pipelines/deploy-fbc-operator/0.2/README.MD[23-36]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The `log-non-push-tasks` entry in the pipeline flow list is formatted differently than the rest (missing the `(<task-name>)` pattern).

### Issue Context
Other tasks in the same list use the pattern `**Title (`task-name`)**`, but `log-non-push-tasks` is rendered as `**Log Non Push Tasks `log-non-push-tasks`**`.

### Fix Focus Areas
- pipelines/deploy-fbc-operator/0.1/README.MD[16-18]
- pipelines/deploy-fbc-operator/0.2/README.MD[32-34]

### Proposed change
Update the entry to match the existing convention, e.g.:
- `3. **Log Non Push Tasks (`log-non-push-tasks`)**` (and similarly in v0.2)
Optionally add a trailing period to the description line(s) for consistency with the rest of the section.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context

✅ Compliance rules (platform): 46 rules

✅ Skills: ci-cd-quirks, running-task-tests, pr-definition-of-done

Tip of the day

💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous review results

Review updated until commit 5ac024c

Results up to commit e81d952

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Validation task unbound vars ✓ Resolved 🐞 Bug ≡ Correctness

Description

validate-parameters reads ${SNAPSHOT}, ${PACKAGE_NAME}, etc. under set -euo pipefail but
does not define task params or step env bindings, so the script will fail with an unbound-variable
error before emitting the intended validation output. This can block the entire pipeline at the very
first task.

Code

pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[R98-129]

+              #!/usr/bin/env bash
+              set -euo pipefail
+
+              if [[ -z "${SNAPSHOT}" ]]; then
+                echo "SNAPSHOT parameter is required"
+                exit 1
+              fi
+
+              if [[ -z "${PACKAGE_NAME}" ]]; then
+                echo "PACKAGE_NAME parameter is empty: the step will discover the package name in the FBC fragment."
+              fi
+
+              if [[ -z "${CHANNEL_NAME}" ]]; then
+                echo "CHANNEL_NAME parameter is empty. The step will determine the default channel name of the selected package."
+              fi
+
+              if [[ -z "${CREDENTIALS_SECRET_NAME}" ]]; then
+                echo "CREDENTIALS_SECRET_NAME parameter is required"
+                exit 1
+              fi
+
+              if [[ -z "${OCI_REF}" ]]; then
+                echo "OCI_REF parameter is required"
+                exit 1
+              fi
+
+              if [[ -z "${REPO_TOKEN}" &&  -z "${REPO_KEY}" ]]; then
+                echo "WARNING: REPO_TOKEN and REPO_KEY parameters are not set."
+              elif [[ -z "${REPO_TOKEN}" || -z "${REPO_KEY}" ]]; then
+                echo "REPO_TOKEN and REPO_KEY should be set together."
+                exit 1
+              fi

Relevance

●●● Strong

Team fixes pipeline-breaking bash issues under set -euo pipefail (accepted in PR #279); similar
deploy-fbc-operator pipeline bugs fixed (PR #215).

PR-#279
PR-#215

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new validate-parameters task uses multiple variables as bash environment variables while `set
-u` is enabled, but it does not bind the corresponding Tekton pipeline parameters into the step
environment, meaning those $VAR references will be unset and trigger an immediate abort. Other
inline steps in the same pipeline demonstrate the expected repository pattern by explicitly mapping
$(params.X) into env: before referencing $X in bash, highlighting that this task is missing
that required wiring.

pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[92-137]
pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[138-152]
pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[635-662]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[94-139]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[140-154]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[647-674]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The inline `validate-parameters` task uses bash variables like `$SNAPSHOT`/`$OCI_REF` while running with `set -u`. Since Tekton pipeline params are not automatically available as shell environment variables, the step will error with an "unbound variable" and never reach the intended `echo`/`exit 1` validation messages.

## Issue Context
Other inline steps in this same pipeline explicitly map `$(params.X)` into `env:` before using `$X` in bash scripts, which is the expected pattern in this repo.

## Fix Focus Areas
- pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[92-152]
- pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[94-154]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit 0daa657

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Inconsistent task name formatting ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The new README task list entry for log-non-push-tasks omits the parenthesized task-name style used
by the other entries (e.g., Parse Metadata (parse-metadata)), reducing consistency/scanability
and making task-name copy/paste patterns uneven. The same inconsistency appears in both the 0.1 and
0.2 pipeline READMEs.

Code

pipelines/deploy-fbc-operator/0.1/README.MD[R16-18]

+3. **Log Non Push Tasks `log-non-push-tasks`**
+   Informs which tasks are going to be skipped for non `push` events, such as
+   `pull_request` or `retest-all-comment`

Relevance

●● Moderate

No prior accepted/rejected reviews found enforcing README task-list “Title (task)” formatting;
only unrelated README feedback exists.

PR-#180
PR-#228
PR-#276

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Both READMEs use the **Title (task-name)** convention for other tasks, but the newly added
log-non-push-tasks entry deviates from that convention.

pipelines/deploy-fbc-operator/0.1/README.MD[7-20]
pipelines/deploy-fbc-operator/0.2/README.MD[23-36]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The `log-non-push-tasks` entry in the pipeline flow list is formatted differently than the rest (missing the `(<task-name>)` pattern).

### Issue Context
Other tasks in the same list use the pattern `**Title (`task-name`)**`, but `log-non-push-tasks` is rendered as `**Log Non Push Tasks `log-non-push-tasks`**`.

### Fix Focus Areas
- pipelines/deploy-fbc-operator/0.1/README.MD[16-18]
- pipelines/deploy-fbc-operator/0.2/README.MD[32-34]

### Proposed change
Update the entry to match the existing convention, e.g.:
- `3. **Log Non Push Tasks (`log-non-push-tasks`)**` (and similarly in v0.2)
Optionally add a trailing period to the description line(s) for consistency with the rest of the section.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit b746351

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Non-push runs fail early ✗ Dismissed 🐞 Bug ≡ Correctness

Description

validate-parameters unconditionally exits when CREDENTIALS_SECRET_NAME or OCI_REF is empty,
even though the tasks that consume those values only run for test-event-type == push. This will
cause pull_request/retest PipelineRuns to fail at the first task instead of cleanly skipping
provisioning/deploy tasks.

Code

pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[R115-123]

+              if [[ -z "${params.CREDENTIALS_SECRET_NAME}" ]]; then
+                echo "CREDENTIALS_SECRET_NAME parameter is required"
+                exit 1
+              fi
+
+              if [[ -z "${params.OCI_REF}" ]]; then
+                echo "OCI_REF parameter is required"
+                exit 1
+              fi

Relevance

●● Moderate

No prior precedent on conditional required params by event; only optional-param gating seen in
PR276/277.

PR-#276
PR-#277

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new validation task exits if CREDENTIALS_SECRET_NAME/OCI_REF are empty, but
deploy-operator (which mounts the credentials secret and uses OCI_REF to push artifacts) is
explicitly gated to run only on push events, so these values are not required for non-push flows.

pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[92-138]
pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[561-570]
pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[601-606]
pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[972-987]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[94-140]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[573-582]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`validate-parameters` currently enforces `CREDENTIALS_SECRET_NAME` and `OCI_REF` as required for *all* PipelineRuns, but these are only needed on the push-only execution path. As a result, non-push events (e.g. `pull_request`, `retest-all-comment`) will fail immediately, defeating the purpose of adding `log-non-push-tasks`.

## Issue Context
Downstream tasks that mount/use these parameters are gated with `when: test-event-type in ["push", "Push"]`.

## Fix Focus Areas
- pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[92-139]
- pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[561-606]
- pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[972-987]
- pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[94-141]
- pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[573-619]
- pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[975-991]

## What to change
1. Move the validation that depends on push-only params (e.g., `CREDENTIALS_SECRET_NAME`, `OCI_REF`, and any other deploy-only inputs) to a task that runs **after** `parse-metadata`.
2. Gate those checks with a `when` clause identical to the push-only tasks (i.e., only validate them when `$(tasks.parse-metadata.results.test-event-type)` is `push`).
3. Keep only universally-required checks (e.g., `SNAPSHOT`) in the initial validation task if you still want a first-task fast-fail.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit 938ce20

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Slack link depends on parse-metadata 🐞 Bug ☼ Reliability

Description

parse-metadata now runs after validate-parameters, so any early validation failure prevents
parse-metadata from producing application-name; the finally Slack notification still
interpolates $(tasks.parse-metadata.results.application-name), making the log URL unusable exactly
when validation is supposed to improve debuggability.

Code

pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[R130-131]

+      runAfter:
+        - validate-parameters

Relevance

●● Moderate

PR #277 added Slack log link using parse-metadata result; no precedent addressing missing results on
early validation failure.

PR-#277
PR-#276

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR adds a hard dependency so parse-metadata cannot run if validate-parameters fails, while
the Slack finally task still builds its Konflux UI link using parse-metadata’s application-name
result.

pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[92-133]
pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[109-117]
pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[1013-1040]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[94-145]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[111-119]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[1015-1042]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`parse-metadata` is now gated behind `validate-parameters`, but the `finally.slack-webhook-notification` message still depends on `$(tasks.parse-metadata.results.application-name)`. If validation fails early, that result is not produced, so the Slack message cannot construct a valid Konflux UI URL.

## Issue Context
This PR intentionally adds early-failing validation to avoid silent/stuck pipelines. Early failure paths should still keep failure notifications and log links functional.

## Fix Focus Areas
- pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[129-133]
- pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[1013-1042]
- pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[131-135]
- pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[1016-1042]

## What to change
Implement one of the following (preferred options first):
1. **Make the Slack link independent of `parse-metadata`** (recommended): remove `/applications/$(tasks.parse-metadata.results.application-name)` from the URL and use a URL form that only relies on `$(context.pipelineRun.namespace)` and `$(context.pipelineRun.name)`.
2. **Provide an explicit pipeline param for application name** (e.g., `APPLICATION_NAME`) and use that in the Slack message, so it exists even if `parse-metadata` never runs.
3. If the link must remain based on `parse-metadata`, then **ensure `parse-metadata` runs before validation** (but that likely conflicts with the goal of validating first).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[JAVGan]

@nmars @asergienk can you please check again?

[qodo-app-for-konflux-ci]

Code review by qodo was updated up to the latest commit 0daa657

[qodo-app-for-konflux-ci]

Code review by qodo was updated up to the latest commit b746351

[qodo-app-for-konflux-ci]

Code review by qodo was updated up to the latest commit b746351

[qodo-app-for-konflux-ci]

Code review by qodo was updated up to the latest commit 938ce20

[asergienk]

@JAVGan just small comments, otherwise lgtm! Was this PR tested on a cluster?

[JAVGan]

@JAVGan just small comments, otherwise lgtm! Was this PR tested on a cluster?

No, it was not yet.

@nmars can you please help me on testing this? I'll push some small fixes on README but the core logic will be the same

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 5:30 PM UTC · Completed 5:37 PM UTC
Commit: ec21706 · View workflow run →

[qodo-app-for-konflux-ci]

Code review by qodo was updated up to the latest commit c5780fe

[asergienk]

@ashwgit could you please address the comments from Qodo? If some of them are no applicable and you think they shouldn't be implemented could you please make a comment about that here? Other than that, everything looks good.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:28 PM UTC · Completed 5:47 PM UTC

Commit: 701e62a · View workflow run →

[qodo-app-for-konflux-ci]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. Bash flags not compliant ✗ Dismissed 📜 Skill insight ☼ Reliability

Description

Embedded bash scripts enable strict mode with set -euo pipefail instead of the required long-form
set -o errexit -o nounset -o pipefail. This violates the repository’s mandated strict-mode format
and may cause PR compliance checks to fail.

Code

pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[R98-100]

+              #!/usr/bin/env bash
+              set -euo pipefail
+

Relevance

●●● Strong

Repo has accepted enforcing long-form bash strict mode for inline scripts (compliance requirement).

PR-#312

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2774 mandates that bash scripts include set -o errexit -o nounset -o pipefail
near the top of the script. The cited inline bash scripts in the deploy-fbc-operator pipeline
definitions use set -euo pipefail at the referenced locations, demonstrating they do not match the
prescribed long-form format and therefore fail the compliance requirement.

pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[98-100]
pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[156-158]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[100-102]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[158-160]
Skill: pr-definition-of-done

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Bash strict mode is currently enabled using `set -euo pipefail` in embedded/inline scripts, but the compliance rule requires the explicit long-form flags `set -o errexit -o nounset -o pipefail`.

## Issue Context
PR Compliance ID 2774 requires bash scripts to include `set -o errexit -o nounset -o pipefail` near the top. This issue occurs in newly added inline bash scripts in the `deploy-fbc-operator` pipeline definitions across multiple versions.

## Fix Focus Areas
- pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[98-100]
- pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[156-158]
- pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[100-102]
- pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[158-160]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Misleading optional-param warnings 🐞 Bug ◔ Observability

Description

validate-parameters emits WARNINGs when REPO_TOKEN/REPO_KEY and SLACK_SECRET_NAME are empty,
even though the pipeline parameter docs describe those empty values as valid configurations (public
repo access; Slack notifications intentionally disabled). This adds noisy/misleading output and can
distract users from real misconfigurations.

Code

pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[R119-123]

+              if [[ -z "$(params.REPO_TOKEN)" &&  -z "$(params.REPO_KEY)" ]]; then
+                echo "WARNING: REPO_TOKEN and REPO_KEY parameters are not set."
+              elif [[ -z "$(params.REPO_TOKEN)" || -z "$(params.REPO_KEY)" ]]; then
+                echo "REPO_TOKEN and REPO_KEY should be set together."
+                exit 1

Relevance

●● Moderate

Subjective/noisy-warning change; PR intent adds warnings for missing params; no close precedent
about removing them.

PR-#276

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The pipeline params define empty values for repo credentials and Slack secret name as
valid/expected, but the newly added validation task prints WARNINGs when those values are empty,
creating a mismatch between documented behavior and emitted logs.

pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[60-75]
pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[119-128]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[60-75]
pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[121-130]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new `validate-parameters` step logs WARNINGs for intentionally-empty optional parameters (`REPO_TOKEN`/`REPO_KEY`, `SLACK_SECRET_NAME`). This conflicts with the parameter semantics (optional/public repo; Slack disabled when empty) and creates misleading/noisy logs.

### Issue Context
- `REPO_TOKEN`/`REPO_KEY` are optional (defaults to empty) and only needed for private repos.
- `SLACK_SECRET_NAME` explicitly supports being empty to disable Slack notifications.

### Fix Focus Areas
- pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml[119-128]
- pipelines/deploy-fbc-operator/0.2/deploy-fbc-operator.yaml[121-130]

### Suggested change
- Replace the two WARNING messages with INFO messages that explain the intended behavior, e.g.:
 - `INFO: REPO_TOKEN/REPO_KEY not set; assuming public repo access.`
 - `INFO: SLACK_SECRET_NAME is empty; Slack notifications are disabled.`
- Keep the existing *error* behavior for the partially-specified repo credential case (only one of the two set), since that is a real misconfiguration.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context

✅ Compliance rules (platform): 46 rules

✅ Skills: ci-cd-quirks, running-task-tests, pr-definition-of-done

Tip of the day

💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[fullsend-ai-review]

Review

Findings

Low

• [injection] pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml — The validate-parameters task interpolates pipeline parameters directly into a bash script: block via $(params.*). While the attack surface is minimal (parameters come from PAC automation, not end-user input), defense-in-depth recommends using env: blocks, consistent with the pattern used by other steps in the same pipeline (e.g., retrieve-auth-header). Same pattern in 0.2.

• [injection] pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml — The log-non-push-tasks task interpolates $(tasks.parse-metadata.results.test-event-type) directly into a bash echo statement. Same recommendation: use an env: block for defense-in-depth. Same pattern in 0.2.

• [edge-case] pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml — The validate-parameters task does not validate the SNAPSHOT parameter, which is required (no default) and is the primary input to parse-metadata. Tekton rejects missing required params, but an explicitly empty string would reach parse-metadata without a clear validation error. Same applies to 0.2.

• [error-message-format] pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml — Inconsistent message prefixes in validate-parameters: warning messages use WARNING: prefix, but fatal errors before exit 1 have no prefix. Consider adding ERROR: prefix and writing to stderr for consistency. Same in 0.2.

• [maintainability] pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml — The log-non-push-tasks script contains a hardcoded list of 7 task names matching the pipeline's push-gated tasks. This list must be manually kept in sync if tasks are added or removed. Same in 0.2.

• [intent-category-mismatch] PR title uses fix(CLOUDDST-30777) but the changes add new functionality (validation and logging tasks). feat or chore would more accurately categorize this change.

• [script-idiom] pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml — The log-non-push-tasks script uses a bash array (skipped_tasks=(...)) not found elsewhere in codebase inline scripts. Functional but introduces a new idiom. Same in 0.2.

---

Labels: PR adds new pipeline tasks and bumps image versions in the deploy-fbc-operator pipeline

[ashwgit]

@asergienk here's the passing test cases -

1. version 0.1 - https://konflux-ui.apps.stone-stg-rh01.l2vh.p1.openshiftapps.com/ns/iib-tenant/applications/ashwkuma-app/pipelineruns/tekton-integration-catalog-4g9fr?integrationTestName=tekton-integration-catalog
2. version 0.2 - https://konflux-ui.apps.stone-stg-rh01.l2vh.p1.openshiftapps.com/ns/iib-tenant/applications/ashwkuma-app/pipelineruns/deploy-fbc-operator-run-b945x?integrationTestName=integration-test-2

Do let me know if you need anything else before we can merge it.

[ashwgit]

Review

Findings

Low

* **[injection]** `pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml` — The `validate-parameters` task interpolates pipeline parameters directly into a bash `script:` block via `$(params.*)`. While the attack surface is minimal (parameters come from PAC automation, not end-user input), defense-in-depth recommends using `env:` blocks, consistent with the pattern used by other steps in the same pipeline (e.g., `retrieve-auth-header`). Same pattern in 0.2.

* **[injection]** `pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml` — The `log-non-push-tasks` task interpolates `$(tasks.parse-metadata.results.test-event-type)` directly into a bash `echo` statement. Same recommendation: use an `env:` block for defense-in-depth. Same pattern in 0.2.

* **[edge-case]** `pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml` — The `validate-parameters` task does not validate the `SNAPSHOT` parameter, which is required (no default) and is the primary input to `parse-metadata`. Tekton rejects missing required params, but an explicitly empty string would reach `parse-metadata` without a clear validation error. Same applies to 0.2.

* **[error-message-format]** `pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml` — Inconsistent message prefixes in `validate-parameters`: warning messages use `WARNING:` prefix, but fatal errors before `exit 1` have no prefix. Consider adding `ERROR:` prefix and writing to stderr for consistency. Same in 0.2.

* **[maintainability]** `pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml` — The `log-non-push-tasks` script contains a hardcoded list of 7 task names matching the pipeline's push-gated tasks. This list must be manually kept in sync if tasks are added or removed. Same in 0.2.

* **[intent-category-mismatch]** PR title uses `fix(CLOUDDST-30777)` but the changes add new functionality (validation and logging tasks). `feat` or `chore` would more accurately categorize this change.

* **[script-idiom]** `pipelines/deploy-fbc-operator/0.1/deploy-fbc-operator.yaml` — The `log-non-push-tasks` script uses a bash array (`skipped_tasks=(...)`) not found elsewhere in codebase inline scripts. Functional but introduces a new idiom. Same in 0.2.

Labels: PR adds new pipeline tasks and bumps image versions in the deploy-fbc-operator pipeline

---

Injection Issues -

• As the bash commands does not directly comes from user, we can skip this improvement or we can create a separate MR for improving injection related issues.

Edge Case for Snapshot Validation

• I expect snapshots are created after application build and integration test are executed after application build , we expect to always have a snapshot.

Error-message-format

• For the error part, fail fast is more suitable , so I don't see any use of "ERROR" prefix.

Intent-category-mismatch

• The title is more obvious to the fix we are developing.

[ashwgit]

@maneeshmehra I have provided comments for the reviews by fullsend, please have a look and let me know if we need any further improvement for this MR.

[maneeshmehra]

Approved.

[ashwgit]

@konflux-ci/konflux-devprod-team could you please have a look on this MR please?

[Dannyb48]

LGTM