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

Use a common k8s API fake reader #1573

Open
pleshakov opened this issue Feb 13, 2024 · 6 comments · May be fixed by #3018
Open

Use a common k8s API fake reader #1573

pleshakov opened this issue Feb 13, 2024 · 6 comments · May be fixed by #3018
Labels
backlog Currently unprioritized work. May change with user feedback or as the product progresses. good first issue Good for newcomers tech-debt Short-term pain, long-term benefit

Comments

@pleshakov
Copy link
Contributor

#1551 telemetry package starting using https://github.com/nginxinc/nginx-gateway-fabric/blob/dca4d6432ee43403c3b815186b9b8585a2ca8a48/internal/framework/events/eventsfakes/fake_reader.go#L13 which is generated in https://github.com/nginxinc/nginx-gateway-fabric/blob/dca4d6432ee43403c3b815186b9b8585a2ca8a48/internal/framework/events/first_eventbatch_preparer.go#L28 , which creates unnecessary dependency between modules

Acceptance criteria:

  • Remove that unnecessary dependency

Came from #1551 (comment)
@pleshakov pleshakov mentioned this issue Feb 13, 2024
Merged
6 tasks
@github-actions GitHub Actions
Copy link
Contributor

This issue is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale Pull requests/issues with no activity label Feb 28, 2024
@sjberman sjberman added the tech-debt Short-term pain, long-term benefit label Feb 28, 2024
@github-actions github-actions bot removed the stale Pull requests/issues with no activity label Feb 29, 2024
@mpstefan mpstefan changed the title Use a commom k8s API fake reader Use a common k8s API fake reader Mar 11, 2024
@mpstefan mpstefan added good first issue Good for newcomers backlog Currently unprioritized work. May change with user feedback or as the product progresses. labels Mar 11, 2024
@miledxz
Copy link
Contributor

miledxz commented Jan 14, 2025 •
edited
Loading

Hi @mpstefan feel free to assign me !

@pleshakov could you please explain a little bit more in details what is main idea,
from my understanding we want to remove usage of FakeReader generated by counterfeiter, from tests of telemetry pkg ?
As stated here: #1551 (comment)

But what would replace it then ? Feel free to correct me as I'm not sure what is main idea.

Thank you in advance !

@kate-osborn
Copy link
Contributor

Hi @mpstefan feel free to assign me !

@pleshakov could you please explain a little bit more in details what is main idea, from my understanding we want to remove usage of FakeReader generated by counterfeiter, from tests of telemetry pkg ? As stated here: #1551 (comment)

But what would replace it then ? Feel free to correct me as I'm not sure what is main idea.

Thank you in advance !

Hi @miledxz, the idea is to move the FakeReader mock out of the eventfakes package into a common package -- maybe named kubernetesfakes. This package can live in /internal/framework/. All of the users of the FakeReader will now depend on kubernetesfakes instead of eventfakes.

@miledxz
Copy link
Contributor

miledxz commented Jan 14, 2025

Hi @kate-osborn thank you for fast reply, so what I can do is just create another directory named kubernetesfakes and move FakeReader mock there, and then update import paths as required,

just wanted to clarify so we are on the same page, thanks !

@kate-osborn
Copy link
Contributor

Hi @kate-osborn thank you for fast reply, so what I can do is just create another directory named kubernetesfakes and move FakeReader mock there, and then update import paths as required,

just wanted to clarify so we are on the same page, thanks !

Actually, since the mock is generated you will have to do a few other things:

  • create a kubernetes directory in /internal/framework/
  • delete the eventfakes directory
  • move the Reader interface to the kubernetes directory. Add //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate above the interface definition
  • run make generate from the root of the repo
  • update import statements

@miledxz
Copy link
Contributor

miledxz commented Jan 14, 2025

@kate-osborn thank you for all the explanations and your time, will do !

This was referenced Jan 14, 2025
Closed
Open
@sindhushiv sindhushiv modified the milestone: v2.0.0 Jan 15, 2025
@sindhushiv sindhushiv moved this from 🆕 New to 👀 In Review in NGINX Gateway Fabric Jan 15, 2025
@sindhushiv sindhushiv moved this from 👀 In Review to External Pull Requests in NGINX Gateway Fabric Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
backlog Currently unprioritized work. May change with user feedback or as the product progresses. good first issue Good for newcomers tech-debt Short-term pain, long-term benefit
Projects
NGINX Gateway Fabric
Status: External Pull Requests
Milestone
No milestone
6 participants
@pleshakov @mpstefan @sindhushiv @sjberman @kate-osborn @miledxz