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

Ensure evaluators are not destroyed if reused #1532

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

zregvart
Copy link
Member

We changed the life cycle of evaluators to the top of the command, and mistakenly left the destruction of the evaluator tied to the lifecycle of validating a single component.
This causes files created for one evalator and shared between component validations or evaluators inaccessible. An error such as:

get compiler options: capabilities not opened: open /tmp/ec-work-299517100/capabilities.json: no such file or directory

would be reported.

This changes the destruction of the evaluator to be in line with the new lifecycle.

Copy link

codecov bot commented Apr 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.29%. Comparing base (af07c33) to head (6cee4a1).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1532      +/-   ##
==========================================
+ Coverage   80.59%   87.29%   +6.70%     
==========================================
  Files          66       77      +11     
  Lines        4761     5165     +404     
==========================================
+ Hits         3837     4509     +672     
+ Misses        924      656     -268     
Flag Coverage Δ
acceptance 72.88% <100.00%> (?)
generative 80.59% <ø> (ø)
integration 80.59% <ø> (ø)
unit 80.59% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
cmd/validate/image.go 96.45% <100.00%> (+0.18%) ⬆️
internal/image/validate.go 75.96% <ø> (+9.61%) ⬆️

... and 40 files with indirect coverage changes

Copy link
Contributor

@robnester-rh robnester-rh left a comment

Choose a reason for hiding this comment

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

LGTM

We changed the life cycle of evaluators to the top of the command, and
mistakenly left the destruction of the evaluator tied to the lifecycle
of validating a single component.
This causes files created for one evalator and shared between component
validations or evaluators inaccessible. An error such as:

```
get compiler options: capabilities not opened: open /tmp/ec-work-299517100/capabilities.json: no such file or directory
```

would be reported.

This changes the destruction of the evaluator to be in line with the new
lifecycle.
@zregvart zregvart force-pushed the pr/evaluator-lifecycle branch from d9920ab to 6cee4a1 Compare April 18, 2024 10:17
@zregvart
Copy link
Member Author

The acceptance tests were tweaked, they did not fail when I removed the change from the production code because the evaluator was executing fast enough for the issue not to manifest itself. Now each component's image is unique, so evaluation takes a bit more, but enough to trigger the issue.

@zregvart
Copy link
Member Author

/retest

1 similar comment
@lcarva
Copy link
Member

lcarva commented Apr 18, 2024

/retest

@lcarva
Copy link
Member

lcarva commented Apr 18, 2024

Seems like failure is due to a random Tekton resolver error:

Pipeline rhtap-contract-tenant/cli-main-ci-on-pull-request-xxpzv can't be Run; it contains Tasks that don't exist: Couldn't retrieve Task "": retryable error validating referenced object deprecated-image-check: Internal error occurred: failed calling webhook "webhook.pipeline.tekton.dev": failed to call webhook: Post "https://tekton-pipelines-webhook.openshift-pipelines.svc:443/defaulting?timeout=10s": context deadline exceeded

@lcarva
Copy link
Member

lcarva commented Apr 18, 2024

/retest

@lcarva
Copy link
Member

lcarva commented Apr 18, 2024

The Red Hat Konflux / ec-main-ci-enterprise-contract / cli-main-ci check is tracking the PipelineRun ec-main-ci-j46zp-vjp6n. That has already completed successfully, but something prevented the results from getting reported here. Let's go ahead and merge this.

@lcarva lcarva merged commit 91b45b2 into enterprise-contract:main Apr 18, 2024
10 checks passed
@zregvart zregvart deleted the pr/evaluator-lifecycle branch April 19, 2024 09:05
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