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 all References to the internal Directories #217

Closed
wants to merge 2 commits into from

Conversation

luthermonson
Copy link

kind/feature

What this PR does / why we need it:
I'd like to move everything in an internal directory out so they can properly be included by goimports in other projects.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@luthermonson luthermonson force-pushed the drop-internal branch 3 times, most recently from d480c94 to c85e742 Compare December 8, 2023 17:21
@luthermonson luthermonson force-pushed the drop-internal branch 2 times, most recently from d34d070 to 907dcf5 Compare December 8, 2023 21:02
@luthermonson luthermonson changed the title removal all references to the internal directories Remove all References to the internal Directories Dec 8, 2023
@richardcase richardcase added the kind/refactor Denotes a change that is a refactor label Dec 13, 2023
@furkatgofurov7
Copy link
Contributor

Hey @luthermonson thanks for the PR. Can you please rebase it (to v1beta1 API) and we can then give it a review?

@furkatgofurov7 furkatgofurov7 added this to the v0.3.0 milestone Feb 19, 2024
@luthermonson
Copy link
Author

@furkatgofurov7 ya ill get that updated

@luthermonson luthermonson force-pushed the drop-internal branch 2 times, most recently from bf4c728 to a5d0f92 Compare February 23, 2024 19:59
@luthermonson
Copy link
Author

@furkatgofurov7 ready to run workflows

@luthermonson
Copy link
Author

@furkatgofurov7 updated again and ready for workflows to be ran

Copy link
Contributor

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

Overall looks good to me except a small nit below, @richardcase do you have any comments?

@@ -9,7 +9,8 @@
"go.mod",
"go.sum",
"api",
"internal",
"controllers",
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we tried running Tilt with these changes? I assume it was already broken before this PR (I can only see a single pkg folder in the repo which is in the root), but would be good to fix it with the same change since we are touching it anyway

Copy link
Author

Choose a reason for hiding this comment

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

i have not, i just made sure things compiled. not sure i even know what change you'd like me to make? change controllers to bootstrap/controllers ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@luthermonson sorry for not making it clear, I was referring to "pkg" which does not exist inside of neither https://github.com/luthermonson/cluster-api-provider-rke2/tree/drop-internal/bootstrap and https://github.com/luthermonson/cluster-api-provider-rke2/tree/drop-internal/controlplane.

It is not a blocker for this PR and can be done (if it makes sense!) in a follow-up as well.

@furkatgofurov7 furkatgofurov7 modified the milestones: v0.3.0, v0.6.0 Aug 13, 2024
@furkatgofurov7
Copy link
Contributor

@luthermonson would you have time to rebase and resolve the open comment in #217 (comment) ?

@furkatgofurov7 furkatgofurov7 modified the milestones: v0.6.0, v0.7.0 Aug 28, 2024
@furkatgofurov7
Copy link
Contributor

@luthermonson gentle ping to know if you are still interested picking this up back.

@furkatgofurov7
Copy link
Contributor

Feel free to re-open if needed, closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/refactor Denotes a change that is a refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants