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

fix ReportActionStatus panic #576

Merged
merged 3 commits into from
Apr 29, 2022

Conversation

displague
Copy link
Member

Description

Applies the suggested fix from #559 (with a different error message).
I'm not familiar with this code but wanted to get a PR started.

Why is this needed

GRPC server panics when the index is over limit.

Fixes: #559

How Has This Been Tested?

Not tested.

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

@displague displague changed the title Report action status panic fix ReportActionStatus panic Jan 11, 2022
@displague displague force-pushed the report-action-status-panic branch from 6d06134 to 6c0f5f6 Compare January 12, 2022 13:16
@cprivitere
Copy link

@displague Can you take a look at the error and rebase?

@gauravgahlot gauravgahlot force-pushed the report-action-status-panic branch from d5b53d2 to b6b68f4 Compare April 29, 2022 11:27
@gauravgahlot gauravgahlot self-requested a review April 29, 2022 11:29
Signed-off-by: Gaurav Gahlot <[email protected]>
@gauravgahlot gauravgahlot force-pushed the report-action-status-panic branch from b6b68f4 to 1e94ce1 Compare April 29, 2022 11:30
gauravgahlot
gauravgahlot previously approved these changes Apr 29, 2022
@codecov
Copy link

codecov bot commented Apr 29, 2022

Codecov Report

Merging #576 (63fe227) into main (d56d3e9) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

❗ Current head 63fe227 differs from pull request most recent head eed0730. Consider uploading reports for the commit eed0730 to get more accurate results

@@            Coverage Diff             @@
##             main     #576      +/-   ##
==========================================
- Coverage   44.39%   44.37%   -0.03%     
==========================================
  Files          61       61              
  Lines        3489     3491       +2     
==========================================
  Hits         1549     1549              
- Misses       1857     1858       +1     
- Partials       83       84       +1     
Impacted Files Coverage Δ
server/dbserver.go 0.00% <ø> (ø)
server/dbserver_worker_workflow.go 89.92% <0.00%> (-1.32%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d56d3e9...eed0730. Read the comment docs.

@gauravgahlot gauravgahlot self-requested a review April 29, 2022 12:30
@gauravgahlot gauravgahlot merged commit 713d5ff into tinkerbell:main Apr 29, 2022
mergify bot added a commit that referenced this pull request Jul 6, 2022
## Description


This allows the final action in a workflow to successfully report its status and consequently allow a workflow to complete.
This issue: #559 incorrectly describes a fix that was applied in this PR: #576

## Why is this needed



Fixes: tinkerbell/playground#143

## How Has This Been Tested?



Unit tests were added and the sandbox (vagrant virtualbox quickstart) was used to manually test.

## How are existing users impacted? What migration steps/scripts do we need?





## Checklist:

I have:

- [ ] updated the documentation and/or roadmap (if required)
- [x] added unit or e2e tests
- [ ] provided instructions on how to upgrade
@displague displague deleted the report-action-status-panic branch August 15, 2022 17:17
@displague displague added this to the 0.7.0 milestone Aug 15, 2022
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.

[Tink Server] panic: runtime error: index out of range
3 participants