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

Naming: rename setup and cleanup tasks #2184

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kosstennbl
Copy link
Collaborator

@kosstennbl kosstennbl commented Nov 29, 2024

Description

Rename cnf_setup, cnf_cleanup and setup tasks to cnf_install, cnf_uninstall, install_dependencies
Change documentation, specs and code accordingly

Issues:

Refs: #2181

@kosstennbl kosstennbl marked this pull request as draft November 29, 2024 09:47
@kosstennbl
Copy link
Collaborator Author

kosstennbl commented Nov 29, 2024

Based on #2182, should be merged only after it or rebased

@kosstennbl kosstennbl force-pushed the #2181-rename-setup-tasks branch 2 times, most recently from 9739fb9 to a41b684 Compare December 3, 2024 09:39
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.

Needs some small changes

spec/setup_spec.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_installation/install_common.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_manager.cr Outdated Show resolved Hide resolved
src/tasks/workload/compatibility.cr Outdated Show resolved Hide resolved
src/tasks/workload/observability.cr Outdated Show resolved Hide resolved
Konstantin Yarovoy added 2 commits December 18, 2024 15:26
Refs: #2176
Add installation_priority parameter for specification of order
for installation of deployments.

Signed-off-by: Konstantin Yarovoy <[email protected]>
Refs: #2181

Rename cnf_setup, cnf_cleanup and cluster_api_setup, cluster_api_cleanup tasks to
cnf_install, cnf_uninstall and cluster_api_install, cluster_api_uninstall.
Change documentation, specs and code accordingly.

Signed-off-by: Konstantin Yarovoy <[email protected]>
@svteb svteb force-pushed the #2181-rename-setup-tasks branch from a41b684 to ace6b29 Compare December 19, 2024 15:59
@svteb svteb marked this pull request as ready for review December 19, 2024 16:03
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

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.

I see uninstallation was already in the docs, but maybe, removal is better.
Up to you

@@ -134,6 +134,21 @@ deployments:
...
```

##### Deployment priority

To ensure deployments are executed in a specific order, you can use the `priority` parameter. Deployments are processed in ascending order of their `priority` values, starting with the lowest. During uninstallation, the order is reversed, processing from the highest `priority` value to the lowest. If the `priority` parameter is not specified, it defaults to 0.
Copy link
Collaborator

Choose a reason for hiding this comment

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

uninstallation looks a bit weird ? removal ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be honest I feel like the opposite of removal would be addition. I dont see the need to not use the word "uninstallation", it feels like it reads well as is + it actually conforms with the command name that is used. Additionally in helm documentation they also use the words install/uninstall. This applies to all other comments you left.

INSTALL.md Show resolved Hide resolved
INSTALL.md Show resolved Hide resolved
SOURCE_INSTALL.md Show resolved Hide resolved
SOURCE_INSTALL.md Show resolved Hide resolved
@svteb svteb requested a review from martin-mat December 23, 2024 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants