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(ai-proxy): removed compat fields pre-3.7 #13006

Merged
merged 1 commit into from
May 9, 2024
Merged

fix(ai-proxy): removed compat fields pre-3.7 #13006

merged 1 commit into from
May 9, 2024

Conversation

tysoekong
Copy link
Contributor

Add azure fields to removed_fields.lua to address CP/DP version compatibility issue reported at https://konghq.atlassian.net/browse/KAG-4356

Summary

Checklist

  • The Pull Request has tests
  • A changelog file has been added to CHANGELOG/unreleased/kong or adding skip-changelog label on PR if unnecessary. README.md
  • The Pull Request has backports to all the versions it needs to cover
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix https://konghq.atlassian.net/browse/KAG-4356

@@ -125,5 +125,27 @@ return {
zipkin = {
"propagation",
},
ai_proxy = {
Copy link
Member

Choose a reason for hiding this comment

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

similar to my comment here, we should be able to remove logic from checkers now, that makes response_streaming and model.options.upstream_path nil here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm scared to change stuff that's working right now.

Can we chore this for later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, let's do it later

Copy link
Member

Choose a reason for hiding this comment

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

IMHO: since this is covered by tests, removing checkers logic will confirm things are working the right way (i.e. via the removed_fields) rather than through some other logic, so it would feel safer to me

Copy link
Contributor

Choose a reason for hiding this comment

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

if we remove twice, it feels safer to me...

Copy link
Contributor Author

@tysoekong tysoekong May 9, 2024

Choose a reason for hiding this comment

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

I’m scared to remove this now that EE team will test and verify it, in case this package is doing something oddly specific.

I can add to 3.7.1 rollup and make sure it’s still fixed by hand?

@samugi samugi added this to the 3.7.0 milestone May 9, 2024
locao
locao previously approved these changes May 9, 2024
@locao locao dismissed their stale review May 9, 2024 19:03

Possible issues found

Copy link
Contributor

@locao locao left a comment

Choose a reason for hiding this comment

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

Tested and it's OK.

@locao locao merged commit 9565ef6 into master May 9, 2024
30 checks passed
@locao locao deleted the fix/KAG-4356-2 branch May 9, 2024 20:24
@team-gateway-bot
Copy link
Collaborator

Cherry-pick failed for master, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git remote add upstream https://github.com/kong/kong-ee
git fetch upstream master
git worktree add -d .worktree/cherry-pick-13006-to-master-to-upstream upstream/master
cd .worktree/cherry-pick-13006-to-master-to-upstream
git checkout -b cherry-pick-13006-to-master-to-upstream
ancref=$(git merge-base f02d4efc35ff821f66deed7a60e27577865a0e9b 2f708be86283336d3272945daed27f5c9a21bbea)
git cherry-pick -x $ancref..2f708be86283336d3272945daed27f5c9a21bbea

@github-actions github-actions bot added the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label May 9, 2024
@team-gateway-bot
Copy link
Collaborator

Successfully created backport PR for release/3.7.x:

@kikito kikito removed the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label May 14, 2024
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.

6 participants