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

Add override support for Knative Services #908

Merged
merged 6 commits into from
Nov 24, 2020
Merged

Add override support for Knative Services #908

merged 6 commits into from
Nov 24, 2020

Conversation

rafaelfelix
Copy link
Contributor

What this PR does / why we need it: Add override support for Knative Services

Which issue this PR fixes: fixes #838

Special notes for your reviewer:

@CLAassistant
Copy link

CLAassistant commented Oct 14, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@mflendrich mflendrich left a comment

Choose a reason for hiding this comment

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

Hi @rafaelfelix,

Thanks for your contribution!

Could you please add a test under parser_test.go for cases where a Knative Ingress has, and doesn't have, an override annotation?

For reference, you can reuse TestKnativeIngressAndPlugins: create a test like this:

  • Arrange: create a NewFakeStore() with a KnativeIngress and a KongIngress therein,
  • Act: Build(logger, fakestore)
  • Assert: that the resulting KongState object has the expected overrides in place

@rafaelfelix
Copy link
Contributor Author

hi @mflendrich! just pushed the tests as requested. let me know if there's any other case we gotta cover :) thanks

mflendrich
mflendrich previously approved these changes Oct 23, 2020
Copy link
Contributor

@mflendrich mflendrich left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@hbagdi , is there anything we've missed that needs attention? You're more familiar with the issue than I am. If it's good to go, please merge 🔥

@rafaelfelix
Copy link
Contributor Author

@hbagdi, @mflendrich please review again whenever you have time. Thanks

hbagdi
hbagdi previously approved these changes Nov 23, 2020
@hbagdi hbagdi changed the base branch from main to next November 23, 2020 16:50
@hbagdi hbagdi dismissed their stale review November 23, 2020 16:50

The base branch was changed.

@hbagdi
Copy link
Member

hbagdi commented Nov 23, 2020

@rafaelfelix This needs to target the next branch. Could you please rebase your branch on top of next?

@rafaelfelix
Copy link
Contributor Author

@hbagdi just rebased my branch with next and force-pushed. please review again when you have some time.

thank you

Copy link
Member

@hbagdi hbagdi left a comment

Choose a reason for hiding this comment

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

It is interesting to see how a single line patch can add this functionality now.

@hbagdi hbagdi merged commit 310e364 into Kong:next Nov 24, 2020
@rafaelfelix rafaelfelix deleted the issue-838 branch November 24, 2020 16:53
@hbagdi
Copy link
Member

hbagdi commented Dec 1, 2020

Thank you for your contribution. Please fill out the following form to claim your contributor swag:
https://github.com/Kong/kong/blob/master/CONTRIBUTING.md#contributor-t-shirt.

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.

Add override support for Knative Services
4 participants