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.

Sign up for GitHub

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

Jump to bottom

[conformance] Gateway Dynamic Listener Ports #1859

Closed
wants to merge 15 commits into from
Closed

[conformance] Gateway Dynamic Listener Ports #1859

wants to merge 15 commits into from

Conversation

dprotaso
Copy link
Contributor

What type of PR is this?
/area conformance
/kind feature

What this PR does / why we need it:

This adds a conformance tests where listeners with dynamic ports are added

Which issue(s) this PR fixes:

Part of #1842

Does this PR introduce a user-facing change?:

@k8s-ci-robot k8s-ci-robot added area/conformance kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 21, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 21, 2023
@dprotaso dprotaso mentioned this pull request Mar 21, 2023
Closed
@robscott
Copy link
Member

robscott commented Mar 22, 2023 •
edited
Loading

Hey @dprotaso, thanks for the PR! I'm realizing my issue wasn't very clear, but the goal of #1842 was to actually first ensure we'd reached consensus on requirements and added them to the API spec before we added conformance tests to exercise this. I unfortunately did not communicate that very well, sorry!

In general, I think we need to be careful anytime we're adding conformance tests that require behavior that isn't explicitly required by the spec. I think it's always much safer to agree on the requirements we want via API Spec updates and then to add conformance tests once we've reached an agreement.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 22, 2023
@dprotaso
Copy link
Contributor Author

dprotaso commented Mar 22, 2023 •
edited
Loading

@robscott yeah no worries - I just forgot to mark this as a draft - this was done for the purposes of the discussion.

Also FWIW this test passes with Istio (cc @howardjohn) 🎉

@shaneutt shaneutt added this to the v0.7.0 milestone Mar 29, 2023
@shaneutt shaneutt marked this pull request as draft March 29, 2023 14:42
@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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 29, 2023
@dprotaso dprotaso force-pushed the dynamic-multiple-listeners branch from 65a7761 to 3fd0cef Compare April 4, 2023 20:44
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 4, 2023
@dprotaso dprotaso marked this pull request as ready for review April 4, 2023 20:45
@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 Apr 4, 2023
@dprotaso
Copy link
Contributor Author

dprotaso commented Apr 4, 2023

@shaneutt I put this under StandardExtendedFeatures though I'm not sure if this is Experimental

@dprotaso
Copy link
Contributor Author

dprotaso commented Apr 4, 2023

I marked this PR ready for review since this test is behind a flag

@shaneutt shaneutt modified the milestones: v0.7.0, v0.7.1 Apr 6, 2023
shaneutt
shaneutt previously requested changes May 3, 2023
View reviewed changes
conformance/utils/config/listener.go Outdated Show resolved Hide resolved
@shaneutt shaneutt dismissed their stale review May 3, 2023 18:12

Didn't mean to block.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2023
@dprotaso dprotaso force-pushed the dynamic-multiple-listeners branch from 522b0af to 237559b Compare May 25, 2023 19:20
@dprotaso dprotaso force-pushed the dynamic-multiple-listeners branch from 7ade1b0 to 225fc80 Compare June 30, 2023 22:55
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2023
dprotaso added 3 commits June 30, 2023 18:59
@dprotaso
test dialing to a listener port stops working when listeners are removed
c1e0249
@dprotaso
fix govet warnings
9934124
@dprotaso
tweak test structure
1dd084a 
- fix mutation for delete (do a patch)
- flatten structure (remove t.Run) and have the test fail quickly
@shaneutt shaneutt requested review from robscott and howardjohn July 3, 2023 18:12
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2023
@shaneutt
Copy link
Member

shaneutt commented Aug 2, 2023

Since this hasn't been updated in a while, and has some conflicts I'm going to mark it as a draft again for now as this helps us organize our reviews. Once we're updated and ready for review please feel free to set it back to ready again.

@shaneutt shaneutt marked this pull request as draft August 2, 2023 14:48
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 20, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle rotten
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 19, 2024
@dprotaso dprotaso mentioned this pull request Feb 21, 2024
Closed
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youngnick youngnick Awaiting requested review from youngnick

@keithmattix keithmattix Awaiting requested review from keithmattix

@arkodg arkodg Awaiting requested review from arkodg

@LiorLieberman LiorLieberman Awaiting requested review from LiorLieberman

@sunjayBhatia sunjayBhatia Awaiting requested review from sunjayBhatia

@Xunzhuo Xunzhuo Awaiting requested review from Xunzhuo

@mlavacca mlavacca Awaiting requested review from mlavacca

@shaneutt shaneutt Awaiting requested review from shaneutt

@robscott robscott Awaiting requested review from robscott

@howardjohn howardjohn Awaiting requested review from howardjohn

Assignees
No one assigned
Labels
area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dprotaso @robscott @k8s-ci-robot @shaneutt @k8s-triage-robot @howardjohn @mlavacca