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

Refactor pkg/announce package to adhere to the interfaces pattern #3565

Closed
xmudrii opened this issue Apr 9, 2024 · 2 comments · Fixed by #3580
Closed

Refactor pkg/announce package to adhere to the interfaces pattern #3565

xmudrii opened this issue Apr 9, 2024 · 2 comments · Fixed by #3580
Assignees
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.

Comments

@xmudrii
Copy link
Member

xmudrii commented Apr 9, 2024

We follow the pattern of using interfaces to provide an option to mock parts of the implementation, e.g. see https://github.com/kubernetes/release/blob/a00489762c923fe58d7e02ea67e06c54123594e6/pkg/release/archive.go

The pkg/announce package doesn't follow this pattern at the moment and it's hard to write tests for it. We would like to have that tests for this package to make it easier to accept changes such as #3559

To make writing tests possible, we should refactor the package to adhere to the "interfaces pattern" that we use in other packages. This issue assumes only refactoring the package, writing tests is the second step once the package is successfully refactored.

From the high level overview, we need the following:

  • Announce struct that contains announceOptions and announceImpl
  • announceOptions struct that contains options and parameters for relevant functions
  • announceImpl interface that contains functions that should be mocked in testing (e.g. reading files...)
  • functions should be refactored to be have Announce or announceImpl as a receiver (e.g. func (a Announce) ... or func (impl announceImpl) ...)
@k8s-ci-robot k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority labels Apr 9, 2024
@xmudrii xmudrii added kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Apr 9, 2024
@k8s-ci-robot k8s-ci-robot removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority labels Apr 9, 2024
@embik
Copy link
Member

embik commented Apr 9, 2024

I'd be interested in working on this.

@xmudrii
Copy link
Member Author

xmudrii commented Apr 9, 2024

Thank you! I'll assign the issue to you
/assign @embik

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants