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

Moves ./controllers into ./internal and ./main.go into ./cmd #222

Merged
merged 3 commits into from
May 30, 2023

Conversation

m1kola
Copy link
Member

@m1kola m1kola commented May 19, 2023

Description

  • Moves ./controllers into ./internal
    Unless we have intention to export ./controllers as a library - it makes sense to move this package into ./internal. If we want to expose ./controllers at some point - I would suggest moving it into ./pkg.
  • Moves ./main.go into ./cmd
    We want to add more commands into the repo (Dependency Library + CLI #206, for example) so would be great to avoid having one of the entry points in the repo root

Note: go-apidiff will complain since we no longer export stuff from ./controllers.

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 19, 2023
@m1kola m1kola marked this pull request as ready for review May 19, 2023 09:52
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 19, 2023
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2023
Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
@tmshort
Copy link
Contributor

tmshort commented May 26, 2023

I question moving main.go. As #206 could be done in the deppy repo (or even the kubectl-operator repo), and not necessarily in this repo.
I agree we probably don't need to export most (if any) of operator-controller now, but maybe in the future?

@everettraven
Copy link
Contributor

The actual move looks fine to me, but I agree with @tmshort that we may not need to export anything now, but we could in the future. I think this gets into the discussion of the different supported use cases and form factors that OLMv1 could come in and is probably a discussion much to large for this PR.

That being said, if we find in the future that some stuff needs to be exported we could always refactor it again

@m1kola
Copy link
Member Author

m1kola commented May 26, 2023

I question moving main.go. As #206 could be done in the deppy repo (or even the kubectl-operator repo), and not necessarily in this repo.

@tmshort I would still suggest to move it to cmd as it is very command to place entry points in cmd in Go projects.

agree we probably don't need to export most (if any) of operator-controller now, but maybe in the future?

That being said, if we find in the future that some stuff needs to be exported we could always refactor it again

@tmshort @everettraven I think it is easier to export something than remove publicly available API. I'm in favour of starting with as few exported things as possible and export as the need arises.

@everettraven
Copy link
Contributor

I'm in favour of starting with as few exported things as possible and export as the need arises.

@m1kola I think that is reasonable

@tmshort
Copy link
Contributor

tmshort commented May 26, 2023

I'm in favour of starting with as few exported things as possible and export as the need arises.

@m1kola I think that is reasonable

According to Spock, "I was not attempting to evaluate its moral implications, Doctor. As a matter of cosmic history, it has always been easier to destroy than to create." but not in the case of APIs.

So, yeah.

@m1kola m1kola merged commit 5ba4327 into operator-framework:main May 30, 2023
@m1kola m1kola deleted the repo_structure branch May 30, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants