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

Requiring "Programmed" condition in conformance tests #1732

Merged
merged 1 commit into from
Feb 16, 2023

Conversation

robscott
Copy link
Member

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

What this PR does / why we need it:
This adds a requirement in conformance tests that Gateways must be "Programmed" (previously this was just "Accepted"). Note that this only covers the top level "Programmed" condition, not each Listener.

Which issue(s) this PR fixes:
Fixes #1722

Does this PR introduce a user-facing change?:

Conformance: Gateways must publish the "Programmed" condition.

@k8s-ci-robot k8s-ci-robot added kind/test area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 15, 2023
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 15, 2023
@robscott
Copy link
Member Author

I believe that every time we make this check in conformance tests, we're also expecting the Gateway(s) to be programmed. If I've missed anything, let me know. I'm going to put a hold on this because I'd like to get sign off from a few people before merging. I know this will completely break conformance tests for some implementations because it's a change at such a high level.

/hold
/cc @arkodg @howardjohn @mlavacca @skriss @sunjayBhatia

@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 Feb 15, 2023
@k8s-ci-robot
Copy link
Contributor

@robscott: GitHub didn't allow me to request PR reviews from the following users: arkodg.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

I believe that every time we make this check in conformance tests, we're also expecting the Gateway(s) to be programmed. If I've missed anything, let me know. I'm going to put a hold on this because I'd like to get sign off from a few people before merging. I know this will completely break conformance tests for some implementations because it's a change at such a high level.

/hold
/cc @arkodg @howardjohn @mlavacca @skriss @sunjayBhatia

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.

@arkodg
Copy link
Contributor

arkodg commented Feb 16, 2023

@robscott from a quick glance it looks like this case is being addressed in Envoy Gateway here
you could try testing it out using steps outlined here

Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

This seems like the right change to me, but definitely would like to invite some more people to check this out before we merge it.

@shaneutt shaneutt added this to the v0.6.2 milestone Feb 16, 2023
@shaneutt shaneutt added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Feb 16, 2023
Copy link
Member

@mlavacca mlavacca 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 looks good to me 👍

Copy link
Contributor

@skriss skriss left a comment

Choose a reason for hiding this comment

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

Thanks @robscott, this mostly looks good from Contour's perspective; unfortunately it's causing us to start failing the gateway-observed-generation-bump test because we don't yet support the addition of the second HTTP Listener port, but that's an issue for us rather than the conformance test.

conformance/utils/kubernetes/helpers.go Outdated Show resolved Hide resolved
conformance/utils/kubernetes/helpers.go Outdated Show resolved Hide resolved
@robscott
Copy link
Member Author

Thanks everyone! Sounds like this is safe enough to remove the hold. Next LGTM will merge this.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 16, 2023
Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 16, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mlavacca, robscott, shaneutt

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

@k8s-ci-robot k8s-ci-robot merged commit fc14f7e into kubernetes-sigs:main Feb 16, 2023
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. area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/test lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add missing GatewayConditionType conformance test
6 participants