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 F5 Networks VirtualServer source #3162

Merged

Conversation

mikejoh
Copy link
Contributor

@mikejoh mikejoh commented Nov 16, 2022

Description
This PR adds support for the F5 Networks VirtualServer CRD as a source of creating endpoints.

Partly fixes #2739 since this PR adds support for only the VirtualServer CRD and not the TransportServer one. I believe it make sense to keep these separated in terms of PRs.

Checklist

  • Unit tests updated
  • End user documentation updated
  • Squash commits

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 16, 2022
@mikejoh mikejoh changed the title Add f5 virtualserver source Add F5 Networks VirtualServer source Nov 16, 2022
@mikejoh mikejoh marked this pull request as ready for review November 16, 2022 10:59
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 16, 2022
@stevehipwell
Copy link
Contributor

@mikejoh I can't review your actual code here but as the Helm chart maintainer could I request that you remove the chart changes from this PR and submit them once the new source has been merged? The binary and Helm release lifecycles aren't aligned so it doesn't make sense having them in a single PR.

@mikejoh
Copy link
Contributor Author

mikejoh commented Nov 22, 2022

@mikejoh I can't review your actual code here but as the Helm chart maintainer could I request that you remove the chart changes from this PR and submit them once the new source has been merged? The binary and Helm release lifecycles aren't aligned so it doesn't make sense having them in a single PR.

Check! Fixed here: 7f3c4ed. I'll create a new PR when this one is merged.

@mikejoh
Copy link
Contributor Author

mikejoh commented Nov 22, 2022

/retest

@k8s-ci-robot
Copy link
Contributor

@mikejoh: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@stevehipwell
Copy link
Contributor

/ok-to-test
/retest

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Nov 24, 2022
@mikejoh
Copy link
Contributor Author

mikejoh commented Nov 24, 2022

@stevehipwell Do you have any idea what's going on with those failing tests in the Cloudflare provider? Might be a regression bug? Issue: #3179

@stevehipwell
Copy link
Contributor

@mikejoh sorry I don't know. You could push a minor change (e.g. git commit --amend --no-edit && git push --force-with-lease) to re-trigger the tests as the Prow command has no impact on GH Actions.

@mikejoh
Copy link
Contributor Author

mikejoh commented Nov 24, 2022

@mikejoh sorry I don't know. You could push a minor change (e.g. git commit --amend --no-edit && git push --force-with-lease) to re-trigger the tests as the Prow command has no impact on GH Actions.

@stevehipwell Great, thanks, good to know! 👍🏻 I'll wait a while until the tests are fixed.

@mikejoh
Copy link
Contributor Author

mikejoh commented Nov 29, 2022

/retest

@mikejoh
Copy link
Contributor Author

mikejoh commented Dec 15, 2022

@seanmalloy Hi! 👋, would it be possible for someone to take a look at this PR? I can understand, by looking at the amount of PRs and issues, that all maintainers here have a lot to do!

@mikejoh mikejoh force-pushed the add-f5-virtualserver-source branch 2 times, most recently from df9591d to 2dccc66 Compare December 19, 2022 13:46
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2023
@mikejoh mikejoh force-pushed the add-f5-virtualserver-source branch 2 times, most recently from df9591d to 91416e3 Compare January 10, 2023 12:39
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2023
@mikejoh
Copy link
Contributor Author

mikejoh commented Feb 9, 2023

@szuecs Hi! 👋, would it be possible to take a look at this PR? I've understood that a lot of work is going into the plugin provider and that reviewing PRs adding/changing providers has been postponed. How is it with PRs and code touching Source's?

Thanks!

@adubkov
Copy link

adubkov commented Feb 19, 2023

Would love to have this PR merged.

@szuecs
Copy link
Contributor

szuecs commented Mar 2, 2023

Thanks for the pr but we won't review no merge any new providers. Providers will be enabled to use a webhook. Pleas check #3063 , you can adapt your code to integrate with this interface and it will work.

@mikejoh
Copy link
Contributor Author

mikejoh commented Mar 2, 2023

Thanks for the pr but we won't review no merge any new providers. Providers will be enabled to use a webhook. Pleas check #3063 , you can adapt your code to integrate with this interface and it will work.

@szuecs This PR "only" touches the Source side of things here, not a provider. Or have i misunderstood something? Thanks!

@szuecs
Copy link
Contributor

szuecs commented Mar 2, 2023

@mikejoh yeah true I see it now. Sorry I did not check the source yet.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2023
@mikejoh mikejoh force-pushed the add-f5-virtualserver-source branch from b9ee88d to 9a76be0 Compare March 14, 2023 09:25
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2023
@vinylen
Copy link

vinylen commented Mar 16, 2023

Can this be merged? This would be really useful to have

@szuecs
Copy link
Contributor

szuecs commented Mar 16, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikejoh, szuecs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@szuecs
Copy link
Contributor

szuecs commented Mar 16, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 16, 2023
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 16, 2023
@k8s-ci-robot k8s-ci-robot merged commit 6e0b8b3 into kubernetes-sigs:master Mar 16, 2023
@mikejoh mikejoh deleted the add-f5-virtualserver-source branch March 16, 2023 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for F5 VirtualServer and TransportServer
6 participants