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

connectivity: Fix ineffective version check in BGP tests #2712

Merged
merged 1 commit into from
Jul 24, 2024
Merged

connectivity: Fix ineffective version check in BGP tests #2712

merged 1 commit into from
Jul 24, 2024

Conversation

rastislavs
Copy link
Contributor

Multiple WithCondition statements were causing that only the last one was effective, making version checks ineffective. This combines the conditions into a single WithCondition statement per test.

@rastislavs
connectivity: Fix ineffective version check in BGP tests
9c7e246 
Multiple WithCondition statements were causing that only the
last one was effective, making version checks ineffective.
This combines the conditions into a single WithCondition
statement per test.

Signed-off-by: Rastislav Szabo <[email protected]>
@rastislavs rastislavs added the kind/bug Something isn't working label Jul 24, 2024
@rastislavs rastislavs marked this pull request as ready for review July 24, 2024 08:32
@rastislavs rastislavs requested a review from a team as a code owner July 24, 2024 08:32
This was referenced Jul 24, 2024
Closed
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 24, 2024
joestringer
joestringer approved these changes Jul 24, 2024
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Thanks for the fix.

(I wonder if it's worth putting some footgun check into WithCondition() to avoid this override, but we can follow up on that separately cc @cilium/cilium-cli-maintainers )

@michi-covalent michi-covalent mentioned this pull request Jul 24, 2024
Closed
michi-covalent
michi-covalent approved these changes Jul 24, 2024
View reviewed changes
Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

filed #2713

@michi-covalent michi-covalent merged commit 48a912f into cilium:main Jul 24, 2024
13 checks passed
@joestringer joestringer mentioned this pull request Jul 25, 2024
Merged
@BrewTestBot BrewTestBot mentioned this pull request Aug 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harsimran-pabla harsimran-pabla harsimran-pabla approved these changes

@michi-covalent michi-covalent michi-covalent approved these changes

@joestringer joestringer joestringer approved these changes

Assignees
No one assigned
Labels
kind/bug Something isn't working ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rastislavs @joestringer @michi-covalent @harsimran-pabla