Flatten & unify component structure #961
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Omrigan
reviewed
Jun 23, 2024
There was a problem hiding this comment.
This move of neonvm files into common pkg/ (vs keeping it in neonvm/pkg) hints towards erasing the boundary between neonvm and autoscaling. Personally, I am for that change, as I believe it might eventually help us to simplify the architecture and reduce the number of components.
But, we should agree on this explicitly.
There was a problem hiding this comment.
I'm +1 for "erasing the boundary between neonvm and autoscaling", so probably it's a consensus already :)
sharnoff
force-pushed
the
sharnoff/flatten-components
branch
from
878304e to
1a77f56
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Omrigan
reviewed
Aug 29, 2024
README-NeonVM.md
Outdated
There was a problem hiding this comment.
NeonVM is broader than just the CRD (e.g., neonvm-controller now has its own top-level directory). So I figured that the README for it should be at the highest common directory.
There was a problem hiding this comment.
I guess the reasoning the same as for vxlan controller -> so that there is less stuff in top level dir.
neonvm-vxlan-controller/Dockerfile
Outdated
There was a problem hiding this comment.
I am tempted to suggest neonvm-vxlan-controller -> neonvm/vxlan-controller
There was a problem hiding this comment.
Could you elaborate? (or, why just neonvm-vxlan-controller and not also neonvm-controller ?)
There was a problem hiding this comment.
Because it has an insignificant amount of code and could tucked away from top-level.
Merged
petuhovskiy
approved these changes
Aug 30, 2024
There was a problem hiding this comment.
I'm +1 for "erasing the boundary between neonvm and autoscaling", so probably it's a consensus already :)
sharnoff
force-pushed
the
sharnoff/flatten-components
branch
3 times, most recently
from
86fdd14 to
49cc225
Compare
Omrigan
approved these changes
Sep 7, 2024
sharnoff
force-pushed
the
sharnoff/flatten-components
branch
from
49cc225 to
18ecebb
Compare
sharnoff
force-pushed
the
sharnoff/flatten-components
branch
2 times, most recently
from
97d4462 to
8ecaf63
Compare
This basically encompasses two logical changes: 1. More fully integrating neonvm into the rest of the repository; and 2. Using a consistent directory structure for all components The new structure is: - All components get their own top-level directory (e.g. 'autoscaler-agent', 'neonvm-controller', 'vm-builder') - Inside the directory, there is: - cmd/ - directory containing main.go for the component - Dockerfile - for building the component - *.yaml - various YAML files for deploying, if applicable Additionally, all dependencies outside the 'cmd' package are moved into the 'pkg/' subdirectory. In particular, there is now 'pkg/neonvm/'. Also of note: 'neonvm/hack/kernel' has been relocated to the new top-level directory 'neonvm-kernel/'. Under this structure, the user-visible changes are: 1. Introduction of new YAML: neonvm-controller.yaml 2. Introduction of new YAML: neonvm-vxlan-controller.yaml 3. neonvm.yaml now is *mostly* just the CRD. The primary reasons for this change are: 1. Because it's been long enough since we moved the neonvm repository here that we might as well make it more well-integrated. 2. Because as we look to add components in other languages, we need a directory structure that supports it. ... and now that Autoscaling is GA, we can make the bigger changes we were holding off on :)
sharnoff
force-pushed
the
sharnoff/flatten-components
branch
from
8c4e1c6 to
7daf447
Compare
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
HTML Report |
sharnoff
added a commit
that referenced
this pull request
Oct 10, 2024
sharnoff
added a commit
that referenced
this pull request
Oct 14, 2024
Omrigan
added a commit
that referenced
this pull request
Oct 18, 2024
Since last release, vm-builder directory was moved into root of the repo. Because of this, we can't download vm-builder during release pipeline, this name is already occupied by a dir. This was caused by the following commit: a2f53e3 Flatten & unify component structure (#961) Signed-off-by: Oleg Vasilev <[email protected]>
Omrigan
added a commit
that referenced
this pull request
Oct 18, 2024
This was done in the following commit: a2f53e3 Flatten & unify component structure (#961) Signed-off-by: Oleg Vasilev <[email protected]>
Omrigan
added a commit
that referenced
this pull request
Oct 19, 2024
Since last release, vm-builder directory was moved into root of the repo. Because of this, we can't download vm-builder during release pipeline, this name is already occupied by a dir. This was caused by the following commit: a2f53e3 Flatten & unify component structure (#961) Signed-off-by: Oleg Vasilev <[email protected]>
Omrigan
added a commit
that referenced
this pull request
Oct 19, 2024
This was done in the following commit: a2f53e3 Flatten & unify component structure (#961) Signed-off-by: Oleg Vasilev <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This basically encompasses two logical changes:
The new structure is:
autoscaler-agent,neonvm-controller,vm-builder)cmd/- directory containing main.go for the componentDockerfile- for building the component*.yaml- various YAML files for deploying, if applicableAdditionally, all code outside the
cmdpackage are moved into thepkg/subdirectory. In particular, there is nowpkg/neonvm/.Also of note:
neonvm/hack/kernelhas been relocated to the new top-level directoryneonvm-kernel/.Under this structure, the user-visible changes are:
The primary reasons for this change are:
Notes for review: Splitting the YAMLs will be somewhat tricky to manage safely. I think we should have a clear idea of how we'll do that before merging.
If we go with this approach, we should aim to cut a release soon after merging.