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

AI proxy return unified status in header phase #1588

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

StarryVae
Copy link
Contributor

Ⅰ. Describe what this PR did

AI proxy return unified status in header phase.

#1584 (comment)

@CLAassistant
Copy link

CLAassistant commented Dec 12, 2024

CLA assistant check
All committers have signed the CLA.

@StarryVae StarryVae marked this pull request as draft December 12, 2024 11:55
@StarryVae
Copy link
Contributor Author

should we just return ActionContinue when there is no body here, or sendLocalReply? cc @johnlanni

@johnlanni
Copy link
Collaborator

should we just return ActionContinue when there is no body here, or sendLocalReply? cc @johnlanni

Returning only "ActionContinue" is okay, as this leaves the handling of the response to the backend service.

@StarryVae StarryVae marked this pull request as ready for review December 13, 2024 08:27
@StarryVae
Copy link
Contributor Author

ok, i think it is ready for review.

@johnlanni
Copy link
Collaborator

It seems that all providers no longer need to worry about the return value of the status code. Can we uniformly adjust the interfaces of all providers?

@StarryVae
Copy link
Contributor Author

emm, yeah, i will adjust the OnRequestHeaders of all providers.

@StarryVae
Copy link
Contributor Author

StarryVae commented Dec 13, 2024

image

build docker image: higress-registry.cn-hangzhou.cr.aliyuncs.com/plugins/wasm-go-builder:go1.20.14-tinygo0.29.0-oras1.0.0

should the build image update? @johnlanni

@johnlanni
Copy link
Collaborator

Is your PC an ARM architecture? This image likely does not support ARM architecture

@johnlanni
Copy link
Collaborator

Inspect It seems that it is now a multi-architecture image, and I can also compile under ARM, what is your machine environment?

image

@StarryVae
Copy link
Contributor Author

my environment is just x86

@StarryVae
Copy link
Contributor Author

it can work now, but i have no idea why 😆 , anyway, i can compile now, thanks.

@StarryVae StarryVae force-pushed the ai-proxy-header-phase-fix branch from 2ff427d to 1f5afff Compare December 16, 2024 07:16
@StarryVae
Copy link
Contributor Author

@johnlanni PTAL, thanks.

Copy link
Collaborator

@johnlanni johnlanni left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.42%. Comparing base (ef31e09) to head (2a22b03).
Report is 230 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1588      +/-   ##
==========================================
+ Coverage   35.91%   43.42%   +7.51%     
==========================================
  Files          69       76       +7     
  Lines       11576    12325     +749     
==========================================
+ Hits         4157     5352    +1195     
+ Misses       7104     6636     -468     
- Partials      315      337      +22     

see 69 files with indirect coverage changes

@StarryVae StarryVae force-pushed the ai-proxy-header-phase-fix branch from 1f5afff to 2a22b03 Compare December 16, 2024 09:49
@StarryVae
Copy link
Contributor Author

i have rebased the main branch

@johnlanni johnlanni merged commit 2a200cd into alibaba:main Dec 16, 2024
13 checks passed
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