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

feat(http-log): support http_endpoint field to be referenceable #9714

Merged
merged 3 commits into from
Dec 7, 2022

Conversation

vm-001
Copy link
Contributor

@vm-001 vm-001 commented Nov 8, 2022

Summary

Support http_endpoint field to be referenceable

Full changelog

  • support http_endpoint field to be referenceable

Issue reference

Internal ticket FTI-4483

@vm-001 vm-001 marked this pull request as ready for review November 8, 2022 04:00
@vm-001 vm-001 requested a review from a team as a code owner November 8, 2022 04:00
@vm-001
Copy link
Contributor Author

vm-001 commented Nov 8, 2022

I've not seen any vault test case for the individual plugin that uses referenceable field. So I assume that instead of adding the test case for that, Kong prefers using a single test case for vault feature(https://github.com/Kong/kong/blob/master/spec/02-integration/13-vaults/02-env_vault_spec.lua. (Correct me if I'm wrong). Thus, this PR didn't contain test case for that.

@vm-001 vm-001 added this to the 3.1 milestone Nov 8, 2022
@fffonion
Copy link
Contributor

Discussed with @vm-001 offline, http_endpoint doesn't make much sense to be a secrets and it doesn't
necessary meet the requirements in attached internal tickets. I would suggest to explore adding referencable
to http headers instead.

Copy link
Member

@kikito kikito left a comment

Choose a reason for hiding this comment

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

This change would need to be accompanied by one test

@flrgh
Copy link
Contributor

flrgh commented Nov 17, 2022

Discussed with @vm-001 offline, http_endpoint doesn't make much sense to be a secrets and it doesn't necessary meet the requirements in attached internal tickets. I would suggest to explore adding referencable to http headers instead.

Removing from the 3.1 milestone per this comment. If something changes go ahead and add it back in.

@flrgh flrgh removed this from the 3.1 milestone Nov 17, 2022
@vm-001 vm-001 force-pushed the feat/support-http_endpoint-referenceable branch from 74b07dd to ae174df Compare November 18, 2022 04:52
@vm-001
Copy link
Contributor Author

vm-001 commented Nov 18, 2022

This change would need to be accompanied by one test

Test added. Please let me know if anything else required.

@hanshuebner
Copy link
Contributor

@fffonion @vm-001 Was there a resolution to the discussion regarding making referencable http headers instead of this special-purpose functionality?

@dndx
Copy link
Member

dndx commented Dec 6, 2022

I remember there were discussions that question if it makes sense to make this reference able (and general concern over making too many non-secret configs referenceable).

@vm-001 do you remember the decision on this one?

@hanshuebner
Copy link
Contributor

As per discussion in the planning meeting, we're not going to globally make everything referencable. We're thus going to merge this once tests pass.

@hanshuebner hanshuebner merged commit ffa3fea into master Dec 7, 2022
@hanshuebner hanshuebner deleted the feat/support-http_endpoint-referenceable branch December 7, 2022 11:36
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.

7 participants