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.

Sign up for GitHub

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

Jump to bottom

fix ValidateStepResultsVariables to validate stepResults only #8264

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix ValidateStepResultsVariables to validate stepResults only #8264

wants to merge 1 commit into from

Conversation

jkhelil
Copy link
Member

@jkhelil jkhelil commented Sep 16, 2024 •
edited
Loading

Changes

fixes #8255

There is a mixture of validating step result and task result in both ValidateStepResultsVariables
ValidateStepResultsVariables should use step results set of name to only validate StepResults. TaskResults are validated via the function ValidateTaskResultsVariables

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
  • Has Tests included if any functionality added or changed
  • pre-commit Passed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including functionality, content, code)
  • Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
  • Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
  • Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

@tekton-robot tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 16, 2024
@jkhelil jkhelil force-pushed the fix_validateTaskResultsVariables branch from a7cd2bb to 8e0fa48 Compare September 16, 2024 14:25
@jkhelil
Copy link
Member Author

jkhelil commented Sep 16, 2024

/kind bug

@tekton-robot tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Sep 16, 2024
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1/task_validation.go 98.6% 98.6% -0.0

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1/task_validation.go 98.6% 98.6% -0.0

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1/task_validation.go 98.6% 98.6% -0.0

@jkhelil jkhelil force-pushed the fix_validateTaskResultsVariables branch from 8e0fa48 to 0c6cd04 Compare September 17, 2024 08:06
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign abayer after the PR has been reviewed.
You can assign the PR to them by writing /assign @abayer in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 17, 2024
@jkhelil jkhelil force-pushed the fix_validateTaskResultsVariables branch from 0c6cd04 to 18dee1a Compare September 17, 2024 08:08
@jkhelil jkhelil changed the title fix ValidateStepResultsVariables and validateTaskResultsVariables fix ValidateStepResultsVariables to validate stepResults only Sep 17, 2024
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1/task_validation.go 98.6% 98.6% -0.0

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1/task_validation.go 98.6% 98.6% -0.0

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1/task_validation.go 98.6% 98.6% -0.0

@jkhelil jkhelil force-pushed the fix_validateTaskResultsVariables branch from 18dee1a to 7c5e691 Compare September 17, 2024 08:45
@tekton-robot tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 17, 2024
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1/task_validation.go 98.6% 98.6% -0.0

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1/task_validation.go 98.6% 98.6% -0.0

@jkhelil jkhelil force-pushed the fix_validateTaskResultsVariables branch 2 times, most recently from e078cdd to 98e4c6f Compare September 17, 2024 11:23
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1/task_validation.go 98.6% 98.6% -0.0

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1/task_validation.go 98.6% 98.6% -0.0

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1/task_validation.go 98.6% 98.6% -0.0

@jkhelil jkhelil force-pushed the fix_validateTaskResultsVariables branch from 98e4c6f to f1fc5c5 Compare September 17, 2024 11:53
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage-df to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1/task_validation.go 98.6% 98.6% -0.0

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1/task_validation.go 98.6% 98.6% -0.0

@tekton-robot
Copy link
Collaborator

@jkhelil: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-tekton-pipeline-beta-integration-tests f1fc5c5 link true /test pull-tekton-pipeline-beta-integration-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@vdemeester
Copy link
Member

@jkhelil aside from the validation, can we make sure it works ? (a tests from the reproducer in #8255 that would validate we got the right values).

@jkhelil
Copy link
Member Author

jkhelil commented Sep 18, 2024

@jkhelil aside from the validation, can we make sure it works ? (a tests from the reproducer in #8255 that would validate we got the right values).

The task is been created after validation fix, but, yes i will do the hole test to see if we get the right results

@jkhelil
Copy link
Member Author

jkhelil commented Sep 18, 2024 •
edited
Loading

@vdemeester
I can see it working with the reproducer scenario

➜  tekton tkn tr describe test-ndn9d
Name:              test-ndn9d
Namespace:         default
Task Ref:          collect-data
Service Account:   default
Timeout:           1h0m0s
Labels:
 app.kubernetes.io/managed-by=tekton-pipelines
 tekton.dev/task=collect-data
Annotations:
 pipeline.tekton.dev/release=f1fc5c5

🌡️  Status

STARTED         DURATION    STATUS
3 minutes ago   21s         Succeeded

📝 Results

 NAME           VALUE
 ∙ resultsDir   tom

🦶 Steps

 NAME             STATUS
 ∙ collect-data   Completed
 ∙ reduce         Completed

k get pods
Containers:
  step-collect-data:
    Container ID:  containerd://971efd7afdea2403d40c149e73ee1c534004e9ead4263cf5ae127bc54d1cc083
    Image:         quay.io/konflux-ci/release-service-utils:e633d51cd41d73e4b3310face21bb980af7a662f
    Image ID:      quay.io/konflux-ci/release-service-utils@sha256:13624fbf75efd88c3220aa19b1423786f4cacfd9abab9042e934ff6114addae0
    Port:          <none>
    Host Port:     <none>
    Command:
      /tekton/bin/entrypoint
    Args:
      -wait_file
      /tekton/downward/ready
      -wait_file_content
      -post_file
      /tekton/run/0/out
      -termination_path
      /tekton/termination
      -step_metadata_dir
      /tekton/run/0/status
      -step_results
      singleComponentMode
      -results
      resultsDir
      -entrypoint
      /tekton/scripts/script-0-8v22c
      --
    State:          Terminated
      Reason:       Completed
      Message:      [{"key":"resultsDir","value":"tom","type":1},{"key":"singleComponentMode","value":"scott","type":4},{"key":"StartedAt","value":"2024-09-18T11:31:45.793Z","type":3}]
      Exit Code:    0
      Started:      Wed, 18 Sep 2024 13:31:43 +0200
      Finished:     Wed, 18 Sep 2024 13:31:45 +0200
    Ready:          False
    Restart Count:  0
    Environment:    <none>
    Mounts:
      /tekton/artifacts from tekton-internal-artifacts (rw)
      /tekton/bin from tekton-internal-bin (ro)
      /tekton/creds from tekton-creds-init-home-0 (rw)
      /tekton/downward from tekton-internal-downward (ro)
      /tekton/home from tekton-internal-home (rw)
      /tekton/results from tekton-internal-results (rw)
      /tekton/run/0 from tekton-internal-run-0 (rw)
      /tekton/run/1 from tekton-internal-run-1 (ro)
      /tekton/scripts from tekton-internal-scripts (ro)
      /tekton/steps from tekton-internal-steps (ro)
      /var/run/secrets/kubernetes.io/serviceaccount from kube-api-access-rdjnz (ro)
      /workspace from tekton-internal-workspace (rw)
  step-reduce:
    Container ID:  containerd://0d791fafaa4f65428d3170626c3a53e63873e908aa431cc53ab7832ac70122f3
    Image:         quay.io/konflux-ci/release-service-utils:e633d51cd41d73e4b3310face21bb980af7a662f
    Image ID:      quay.io/konflux-ci/release-service-utils@sha256:13624fbf75efd88c3220aa19b1423786f4cacfd9abab9042e934ff6114addae0
    Port:          <none>
    Host Port:     <none>
    Command:
      /tekton/bin/entrypoint
    Args:
      -wait_file
      /tekton/run/0/out
      -post_file
      /tekton/run/1/out
      -termination_path
      /tekton/termination
      -step_metadata_dir
      /tekton/run/1/status
      -results
      resultsDir
      -entrypoint
      /tekton/scripts/script-1-cf47m
      --
    State:          Terminated
      Reason:       Completed
      Message:      [{"key":"resultsDir","value":"tom","type":1},{"key":"StartedAt","value":"2024-09-18T11:31:45.915Z","type":3}]
      Exit Code:    0
      Started:      Wed, 18 Sep 2024 13:31:43 +0200
      Finished:     Wed, 18 Sep 2024 13:31:47 +0200

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

@abayer abayer Awaiting requested review from abayer

@jerop jerop Awaiting requested review from jerop

Assignees
No one assigned
Labels
kind/bug Categorizes issue or PR as related to a bug. release-note-none Denotes a PR that doesnt merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Validation failed when using spec.results and spec.steps[].results on the same spec
3 participants
@jkhelil @tekton-robot @vdemeester