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

Remove legacy controller & dissimilate internal Go functionality from public API #1591

Closed
5 tasks done
shaneutt opened this issue Jul 27, 2021 · 6 comments
Closed
5 tasks done
Assignees
Labels
area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. priority/medium

Comments

@shaneutt
Copy link
Contributor

shaneutt commented Jul 27, 2021

Problem Statement

We're about to release KIC 2.0 and the maintainers are looking at removing all legacy code from next/main in favor of a branching strategy for the remainder of v1 support time.

Much of our internal functionality used for the controllers is present in publicly link-able packages, potentially putting us on the hook for backwards compatibility and breaking people if we change what we otherwise expect to only be our own controller internals, so we can take this time to also internalize those packages.

Proposed Solution

  • remove all legacy controller, deployment tooling, and CLI code
  • move all functionality that is not our Public API Types or Kubernetes Clients into internal/ to avoid public use in the future

Additional information

We are in a particularly good spot to make this change with KIC 2.0 as it will technically be breaking (albeit we're really hoping nobody is linking to us) and major releases are a good a time as any to pull the cord on such things.

There is already some precedent for making these changes in #1523, we just need to move the legacy code.

Acceptance Criteria

  • a branch has been created off the latest v1 tag for further v1 maintenance
  • railgun/ directory contents moved up, and any code no longer in use removed
  • Golang developers can not utilize our controller internals in other projects
@shaneutt shaneutt added priority/medium area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. labels Jul 27, 2021
@shaneutt shaneutt self-assigned this Jul 27, 2021
@shaneutt shaneutt changed the title Dissimilate internal Go functionality from public API Remove legacy controller & dissimilate internal Go functionality from public API Jul 28, 2021
@mflendrich
Copy link
Contributor

My thoughts:

  • Branching off the v1 controller and cleaning up the v2 codebase is OK
  • I'd reword the Legacy v1 code has been removed, and a branch has been created off the latest v1 release for future maintenance as something like Deleted all code that has been made dead by the removal of the 1.x controller and its tests for more accuracy
  • Golang developers can not utilize our controller internals in other projects sounds very radical to me but I understand the perspective and won't block this.

@mflendrich
Copy link
Contributor

Furthermore, I think that the changes that move packages/files around will likely cause a lot of pain in most (if not all) open PRs.

So let's try to minimize the blast. Maybe we can make the file move in disconnect from dead code deletion, when there isn't much work in flight? Or proactively rebase all open PRs at the time of the move?

@shaneutt
Copy link
Contributor Author

Furthermore, I think that the changes that move packages/files around will likely cause a lot of pain in most (if not all) open PRs.

So let's try to minimize the blast. Maybe we can make the file move in disconnect from dead code deletion, when there isn't much work in flight? Or proactively rebase all open PRs at the time of the move?

I'll opt to proactively rebase 👍

@shaneutt
Copy link
Contributor Author

Branch 1.3.x is the new canonical history for v1 releases, main has been merged in 👍

@ccfishk
Copy link
Contributor

ccfishk commented Jul 28, 2021

No object to the overall strategy , assuming this is part of the duplicate code cleanup work.

A lot of library packages/modules such as pkg/parse, pkg/store etc is heavily shared between legacy controller (< 1.3) and current KIC, after branch, we can either have two copies, or move them to separate repo for maintenance. W/ branches, any fix within the other will require sync to another branch.

The integration tests, currently latest 2.0 branch cover testing legacy image, we can remove them if we will maintain separate 1.3.x and v1/v2 code base.

@shaneutt
Copy link
Contributor Author

No object to the overall strategy , assuming this is part of the duplicate code cleanup work.

A lot of library packages/modules such as pkg/parse, pkg/store etc is heavily shared between legacy controller (< 1.3) and current KIC, after branch, we can either have two copies, or move them to separate repo for maintenance. W/ branches, any fix within the other will require sync to another branch.

The integration tests, currently latest 2.0 branch cover testing legacy image, we can remove them if we will maintain separate 1.3.x and v1/v2 code base.

Sounds good, and seems to align with everything so far 👍 thanks for the input @ccfishk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/maintenance Cleanup, refactoring, and other maintenance improvements that don't change functionality. priority/medium
Projects
None yet
Development

No branches or pull requests

3 participants