-
Notifications
You must be signed in to change notification settings - Fork 50
test: Create namespace "proxy-test" first #1917
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Summary of ChangesHello @Camila-B, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an intermittent test failure in the end-to-end proxy test suite by ensuring a more robust setup for the test environment. Specifically, it modifies the test flow to explicitly create and confirm the readiness of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly addresses a test failure by ensuring the proxy-test namespace is created before other resources are applied within it. The changes involve separating the namespace definition into its own file and updating the test to apply it explicitly. My review includes suggestions to refine the resource application and deletion logic to be more explicit and avoid potential errors during test cleanup.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses a race condition in the TestSyncingThroughAProxy e2e test by ensuring the proxy-test namespace is created and ready before applying other resources into it. The changes involve separating the namespace manifest into its own file, applying it first, and waiting for its status to become current. The test cleanup logic has also been improved to be more robust by explicitly waiting for resource deletion. The file structure under e2e/testdata/proxy has been reorganized to support this, which is a clean solution. The changes are correct and effectively fix the test flakiness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the proxy test setup by extracting the namespace resource into a separate file and updating the test to manage resources with explicit ordering.
Key changes:
- Extracted namespace definition from
tinyproxy.yamlto a dedicatednamespace.yamlfile - Updated test to apply namespace first and wait for it to be ready before applying other resources
- Modified cleanup order to delete configs before deleting the namespace
- Added a new
proxy-enabled.yamlconfiguration file
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| e2e/testdata/proxy/namespace.yaml | New file containing the extracted namespace definition for proxy-test |
| e2e/testdata/proxy/configs/tinyproxy.yaml | Removed namespace definition (now in separate file) |
| e2e/testdata/proxy/configs/proxy-enabled.yaml | New RootSync configuration for testing proxy functionality |
| e2e/testcases/proxy_test.go | Updated to apply namespace separately with wait logic and proper cleanup ordering |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,18 @@ | |||
| # Copyright 2025 Google LLC | |||
Copilot
AI
Oct 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated copyright year to 2025, which is inconsistent with the copyright year (2022) used in other related files like tinyproxy.yaml and proxy-enabled.yaml.
| # Copyright 2025 Google LLC | |
| # Copyright 2022 Google LLC |
Creates namespace
proxy-testfirst before applying the rest of the proxy configurationTest failure: https://oss.gprow.dev/view/gs/oss-prow-build-kpt-config-sync/logs/kpt-config-sync-standard-regular/1982900145926180864