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

Merge all of porch's go modules into a single go module #107

Merged
merged 7 commits into from
Sep 6, 2024

Conversation

kispaljr
Copy link
Collaborator

@kispaljr kispaljr commented Sep 3, 2024

Currently porch consists of three separate go modules:

  • the "root" module that contains primarily the porch server
  • the controllers module that contains the packagevariant* controllers
  • the api module that contains (some of the) porch api definitions in the form of go structs

These modules depend on each other, and that makes it really hard to introduce changes that touches multiple modules.
This PR aims to put all of the above into one singular porch go module to make the lives of developers considerably easier.

@kispaljr
Copy link
Collaborator Author

kispaljr commented Sep 3, 2024

This is still work-in-progress. I will remove the hold label when it is ready for review.

@kispaljr
Copy link
Collaborator Author

kispaljr commented Sep 5, 2024

/retest

1 similar comment
@kispaljr
Copy link
Collaborator Author

kispaljr commented Sep 5, 2024

/retest

@kispaljr
Copy link
Collaborator Author

kispaljr commented Sep 5, 2024

/retest

@kispaljr
Copy link
Collaborator Author

kispaljr commented Sep 5, 2024

/retest

1 similar comment
@kispaljr
Copy link
Collaborator Author

kispaljr commented Sep 5, 2024

/retest

@kispaljr
Copy link
Collaborator Author

kispaljr commented Sep 5, 2024

@efiacor , @liamfallon this is ready for review now

Copy link
Collaborator

@efiacor efiacor left a comment

Choose a reason for hiding this comment

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

Looks great. I'm fully in favour of this approach but could this cause other potential issue that I don't have the golang knowledge to foresee?

pkg/engine/builtinruntime.go Show resolved Hide resolved
go.mod Show resolved Hide resolved
@@ -16,15 +16,6 @@ IMAGE_TAG ?= latest
IMAGE_REPO ?= docker.io/nephio
IMAGE_NAME ?= porch-controllers

# This includes the following targets:
Copy link
Collaborator

Choose a reason for hiding this comment

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

No longer needed?

Copy link
Collaborator

@efiacor efiacor left a comment

Choose a reason for hiding this comment

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

Also, I didn't fully check but we need to ensure that the code-gen stuff is still sound.

@kispaljr
Copy link
Collaborator Author

kispaljr commented Sep 6, 2024

Also, I didn't fully check but we need to ensure that the code-gen stuff is still sound.

I can only state that make generate runs without errors on my machine.

@efiacor
Copy link
Collaborator

efiacor commented Sep 6, 2024

/approve

@nephio-prow nephio-prow bot added the approved label Sep 6, 2024
@kispaljr
Copy link
Collaborator Author

kispaljr commented Sep 6, 2024

Also, after we merge this, we will have to adjust the go.mod files in the Nephio project. (But first, we have to merge this, so that a commit hash is assigned to the new single module)

Copy link
Member

@liamfallon liamfallon left a comment

Choose a reason for hiding this comment

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

I downloaded this PR and it built fine for me

Thanks @kispaljr for this, it'll really help

/approve
/lgtm

Copy link
Contributor

nephio-prow bot commented Sep 6, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: efiacor, kispaljr, liamfallon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nephio-prow nephio-prow bot merged commit 9eca408 into nephio-project:main Sep 6, 2024
5 checks passed
CsatariGergely added a commit to nokia/nephio-docs that referenced this pull request Sep 12, 2024
nephio-prow bot pushed a commit to nephio-project/docs that referenced this pull request Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants