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.

Sign up for GitHub

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

Jump to bottom

installation: CNF Installation (8) Transition to new installation #2171

Merged
merged 1 commit into from
Dec 2, 2024
Merged

installation: CNF Installation (8) Transition to new installation #2171

merged 1 commit into from
Dec 2, 2024

Conversation

kosstennbl
Copy link
Collaborator

Description

Remove old installation method, use the new one instead.
Change specs accordingly.
Remove unnecessary config parameters.
Fully enable multiple deployments installation.

Issues:

Refs: #2169

@kosstennbl kosstennbl marked this pull request as draft November 1, 2024 09:15
@kosstennbl kosstennbl force-pushed the #2169-transition-to-new-installation branch from 2bbd309 to df5f452 Compare November 1, 2024 11:21
@svteb svteb mentioned this pull request Nov 4, 2024
Closed
4 tasks
@kosstennbl kosstennbl force-pushed the #2169-transition-to-new-installation branch 15 times, most recently from 9d66364 to 661e134 Compare November 12, 2024 08:58
@kosstennbl kosstennbl force-pushed the #2169-transition-to-new-installation branch 3 times, most recently from 74f22df to 4251f58 Compare November 13, 2024 21:33
@svteb svteb mentioned this pull request Nov 14, 2024
Merged
20 tasks
svteb
svteb reviewed Nov 14, 2024
View reviewed changes
Copy link
Collaborator

@svteb svteb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

As we've discussed in a meeting, I think that rewriting all mentions of cnf_setup/cnf_cleanup internally to install_cnf/uninstall_cnf might be hard to understand for new contributors. Seeing the state of the code I might be more inclined to switch the cnf_setup/cnf_cleanup to cnf_install/cnf_uninstall on the terminal more now, but I am still not sure (@martin-mat we could use your input on this matter). Additionally I do not think that it is necessary to name the methods install_cnf/uninstall_cnf when they are part of CNFInstall module.

spec/spec_helper.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_manager.cr Show resolved Hide resolved
@kosstennbl kosstennbl force-pushed the #2169-transition-to-new-installation branch from 4251f58 to 2a2d6d1 Compare November 18, 2024 09:52
@kosstennbl kosstennbl mentioned this pull request Nov 18, 2024
Open
@kosstennbl kosstennbl force-pushed the #2169-transition-to-new-installation branch from 2a2d6d1 to 5ee7ca5 Compare November 20, 2024 09:42
svteb
svteb approved these changes Nov 25, 2024
View reviewed changes
Copy link
Collaborator

@svteb svteb left a comment

Choose a reason for hiding this comment

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

lgtm

martin-mat
martin-mat approved these changes Nov 27, 2024
View reviewed changes
Copy link
Collaborator

@martin-mat martin-mat left a comment

Choose a reason for hiding this comment

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

lgtm

collivier
collivier requested changes Nov 27, 2024
View reviewed changes
Copy link
Collaborator

@collivier collivier left a comment

Choose a reason for hiding this comment

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

blank chars + format

spec/5g/core_spec.cr Outdated Show resolved Hide resolved
spec/5g/core_spec.cr Outdated Show resolved Hide resolved
spec/5g/core_spec.cr Outdated Show resolved Hide resolved
spec/5g/core_spec.cr Outdated Show resolved Hide resolved
spec/utils/cnf_manager_spec.cr Show resolved Hide resolved
spec/utils/utils_spec.cr Outdated Show resolved Hide resolved
spec/utils/utils_spec.cr Outdated Show resolved Hide resolved
@collivier collivier force-pushed the #2169-transition-to-new-installation branch from 5ee7ca5 to b4d1a72 Compare November 28, 2024 14:40
@svteb svteb marked this pull request as ready for review November 29, 2024 08:14
new-cnf-installation: Fully transition to new CNF installation method
7ea09f9 
Remove old installation method, use the new one instead.
Change specs accordingly.
Remove unnecessary config parameters.
Fully enable multiple deployments installation.

Refs: #2169
Signed-off-by: Konstantin Yarovoy <[email protected]>
@kosstennbl kosstennbl force-pushed the #2169-transition-to-new-installation branch from b4d1a72 to 7ea09f9 Compare November 29, 2024 16:15
@kosstennbl
Copy link
Collaborator Author

kosstennbl commented Nov 29, 2024 •
edited
Loading

@collivier, Whitespaces and formatting were fixed in mentioned cases, i think that there is still a lot of problems of this type in testsuite, maybe different ticket should be created to fix this.

@kosstennbl kosstennbl requested a review from collivier November 29, 2024 16:28
@collivier collivier merged commit d9575ef into main Dec 2, 2024
166 checks passed
@collivier collivier deleted the #2169-transition-to-new-installation branch December 2, 2024 08:58
This was referenced Dec 3, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svteb svteb svteb approved these changes

@martin-mat martin-mat martin-mat approved these changes

@collivier collivier collivier approved these changes

@denverwilliams denverwilliams Awaiting requested review from denverwilliams denverwilliams is a code owner

@agentpoyo agentpoyo Awaiting requested review from agentpoyo agentpoyo is a code owner

@HashNuke HashNuke Awaiting requested review from HashNuke

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kosstennbl @collivier @martin-mat @svteb