Skip to content

Conversation

miledxz
Copy link
Contributor

@miledxz miledxz commented Jan 14, 2025

Proposed changes

Problem: Adding update of ks client Reader interface and FakeReader into separated directory.

Solution: Moved files, updated import paths.

Closes #1573

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@miledxz miledxz requested a review from a team as a code owner January 14, 2025 20:50
Copy link
Contributor

github-actions bot commented Jan 14, 2025

✅ All required contributors have signed the F5 CLA for this PR. Thank you!
Posted by the CLA Assistant Lite bot.

@github-actions github-actions bot added the tests Pull requests that update tests label Jan 14, 2025
@miledxz
Copy link
Contributor Author

miledxz commented Jan 14, 2025

I have hereby read the F5 CLA and agree to its terms

Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

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

@miledxz changes look good, but it looks like the eventsfakes directory hasn't been deleted. Can you delete it?

@miledxz
Copy link
Contributor Author

miledxz commented Jan 15, 2025

@miledxz changes look good, but it looks like the eventsfakes directory hasn't been deleted. Can you delete it?

Hi @kate-osborn, in eventsfakes dir are other fake mocks like FakeEventHandler and FakeFirstEventBatchPreparer
defined here https://github.com/nginx/nginx-gateway-fabric/blob/main/internal/framework/events/handler.go#L9
and here https://github.com/nginx/nginx-gateway-fabric/blob/main/internal/framework/events/first_eventbatch_preparer.go#L14,

do we need to keep them ? because of other tests

@kate-osborn
Copy link
Contributor

@miledxz changes look good, but it looks like the eventsfakes directory hasn't been deleted. Can you delete it?

Hi @kate-osborn, in eventsfakes dir are other fake mocks like FakeEventHandler and FakeFirstEventBatchPreparer defined here https://github.com/nginx/nginx-gateway-fabric/blob/main/internal/framework/events/handler.go#L9 and here https://github.com/nginx/nginx-gateway-fabric/blob/main/internal/framework/events/first_eventbatch_preparer.go#L14,

do we need to keep them ? because of other tests

ahh, I see! I didn't check for other fakes in that directory. All good, then.

@miledxz
Copy link
Contributor Author

miledxz commented Jan 15, 2025

@miledxz changes look good, but it looks like the eventsfakes directory hasn't been deleted. Can you delete it?

Hi @kate-osborn, in eventsfakes dir are other fake mocks like FakeEventHandler and FakeFirstEventBatchPreparer defined here https://github.com/nginx/nginx-gateway-fabric/blob/main/internal/framework/events/handler.go#L9 and here https://github.com/nginx/nginx-gateway-fabric/blob/main/internal/framework/events/first_eventbatch_preparer.go#L14,
do we need to keep them ? because of other tests

ahh, I see! I didn't check for other fakes in that directory. All good, then.

sounds like a plan !

@miledxz miledxz requested a review from kate-osborn January 15, 2025 19:33
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

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

LGTM!

@sjberman sjberman enabled auto-merge (squash) January 15, 2025 21:35
@sjberman sjberman merged commit b5b8783 into nginx:main Jan 15, 2025
38 checks passed
lucacome pushed a commit that referenced this pull request Jan 16, 2025
Problem: Adding update of ks client Reader interface and FakeReader into separated directory.

Solution: Moved files, updated import paths.
@lucacome lucacome added the tech-debt Short-term pain, long-term benefit label Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech-debt Short-term pain, long-term benefit tests Pull requests that update tests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Use a common k8s API fake reader
4 participants