Skip to content

Conversation

@duguorong009
Copy link
Contributor

@duguorong009 duguorong009 commented Sep 20, 2023

Issue Addressed

Proposed Changes

  • Add Filter::recover to handle rejections specifically as 404 NOT FOUND

Please list or describe the changes introduced by this PR.

Additional Info

Similar to PR #3836

@CLAassistant
Copy link

CLAassistant commented Sep 20, 2023

CLA assistant check
All committers have signed the CLA.

@michaelsproul
Copy link
Member

michaelsproul commented Sep 21, 2023

Looks good. I think you might just need to run cargo fmt --workspace

Lets see if CI passes. Then I'll do some more manual testing, and we can merge.

@michaelsproul michaelsproul added ready-for-review The code is ready for review low-hanging-fruit Easy to resolve, get it before someone else does! labels Sep 21, 2023
@duguorong009
Copy link
Contributor Author

Looks good. I think you might just need to run cargo fmt --workspace

Lets see if CI passes. Then I'll do some more manual testing, and we can merge.

FYI, I ran cargo fmt --all for code.
But, there is no change at all.

Hmm, there is CI failure. I am not much sure why.
Can you let me know what I can do with CI error? @michaelsproul

@michaelsproul
Copy link
Member

We can ignore that failure, it's just a flakiness in the tests. I'll double-check with some manual testing then we can merge

@duguorong009
Copy link
Contributor Author

@michaelsproul @chong-he
Um, can you let me know what stops the merge?
If anything I should do, pls let me know.

@chong-he
Copy link
Member

@michaelsproul @chong-he Um, can you let me know what stops the merge? If anything I should do, pls let me know.

It seems ok to me: 0ba06af

Do you see any error?

As a side note I will be doing some testing on this and hope to get back to you soon

@chong-he
Copy link
Member

Tested both GET and POST HTTP and it is all good. I think we are good to merge this @michaelsproul

Details:

GET:

curl -X GET "http://localhost:5062/lighthouse/testing" -H "Authorization: Bearer api-token-0x02d34c396c87ddcc0690e3f6b8f7fe23fdf5f309f8569fe9147dc3bf5f445db587" | jq

POST:

DATADIR=/var/lib/lighthouse
curl -X POST http://localhost:5062/lighthouse/testing \
-H "Authorization: Bearer $(cat ${DATADIR}/validators/api-token.txt)" \
-H "Content-Type: application/json" \
-d '[
    {
        "enable": true,
        "description": "validator_one",
        "deposit_gwei": "32000000000",
        "graffiti": "Mr F was here",
        "suggested_fee_recipient": "0xa2e334e71511686bcfe38bb3ee1ad8f6babcc03d"
    },
    {
        "enable": false,
        "description": "validator two",
        "deposit_gwei": "34000000000"
    }
]' | jq

With Lighthouse v4.5.0-441fc16 both returned:

{
  "code": 405,
  "message": "METHOD_NOT_ALLOWED",
  "stacktraces": []
}

With this PR Lighthouse v4.5.0-0ba06af both returned:

{
  "code": 404,
  "message": "NOT_FOUND",
  "stacktraces": []
}

Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Great, lets merge this.

Sorry for the delay, we had a merge-freeze in effect while v4.5.0 was happening.

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. v4.6.0 ETA Q1 2024 and removed ready-for-review The code is ready for review labels Sep 29, 2023
@paulhauner
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Sep 29, 2023
…4758)

## Issue Addressed
- Close #4596 

## Proposed Changes
- Add `Filter::recover` to handle rejections specifically as 404 NOT FOUND

Please list or describe the changes introduced by this PR.

## Additional Info

Similar to PR #3836
@bors
Copy link

bors bot commented Sep 29, 2023

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Sep 29, 2023
…4758)

## Issue Addressed
- Close #4596 

## Proposed Changes
- Add `Filter::recover` to handle rejections specifically as 404 NOT FOUND

Please list or describe the changes introduced by this PR.

## Additional Info

Similar to PR #3836
@bors
Copy link

bors bot commented Sep 29, 2023

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Sep 29, 2023
…4758)

## Issue Addressed
- Close #4596 

## Proposed Changes
- Add `Filter::recover` to handle rejections specifically as 404 NOT FOUND

Please list or describe the changes introduced by this PR.

## Additional Info

Similar to PR #3836
@bors
Copy link

bors bot commented Sep 29, 2023

Build failed:

@paulhauner
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Oct 4, 2023
…4758)

## Issue Addressed
- Close #4596 

## Proposed Changes
- Add `Filter::recover` to handle rejections specifically as 404 NOT FOUND

Please list or describe the changes introduced by this PR.

## Additional Info

Similar to PR #3836
@bors
Copy link

bors bot commented Oct 4, 2023

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title fix(validator_client): return http 404 rather than 405 in http api [Merged by Bors] - fix(validator_client): return http 404 rather than 405 in http api Oct 4, 2023
@bors bors bot closed this Oct 4, 2023
@duguorong009 duguorong009 deleted the gr/issue-4596 branch October 4, 2023 01:21
@duguorong009 duguorong009 restored the gr/issue-4596 branch October 4, 2023 01:42
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
…igp#4758)

## Issue Addressed
- Close sigp#4596 

## Proposed Changes
- Add `Filter::recover` to handle rejections specifically as 404 NOT FOUND

Please list or describe the changes introduced by this PR.

## Additional Info

Similar to PR sigp#3836
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
…igp#4758)

## Issue Addressed
- Close sigp#4596 

## Proposed Changes
- Add `Filter::recover` to handle rejections specifically as 404 NOT FOUND

Please list or describe the changes introduced by this PR.

## Additional Info

Similar to PR sigp#3836
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
…igp#4758)

## Issue Addressed
- Close sigp#4596 

## Proposed Changes
- Add `Filter::recover` to handle rejections specifically as 404 NOT FOUND

Please list or describe the changes introduced by this PR.

## Additional Info

Similar to PR sigp#3836
@duguorong009 duguorong009 deleted the gr/issue-4596 branch January 25, 2024 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

low-hanging-fruit Easy to resolve, get it before someone else does! ready-for-merge This PR is ready to merge. v4.6.0 ETA Q1 2024

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants