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

Feature: CNF Installation (4) Group several manifest into one file #2150

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

Conversation

barmull
Copy link
Collaborator

@barmull barmull commented Sep 17, 2024

Description

  • Gather all the manifest files from the various CNFs into a single file called common_manifests.yaml
  • Modified temp_template.yaml to reference common_manifests.yaml
  • Removed code responsible for generating CNF templates as it is no longer used
  • Added functionality in cleanup.cr to remove common_manifests.yaml as part of the cleanup process

Issues:

Refs: #2125

Re-created from different branch, old PR: #2144

src/tasks/utils/cnf_installation/manifest.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_installation/manifest.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_installation/manifest.cr Outdated Show resolved Hide resolved
Copy link
Collaborator

@kosstennbl kosstennbl left a comment

Choose a reason for hiding this comment

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

Nice change!
We should also not forget to modify shard files after PR in Helm repo would be merged.

Some comments and possible changes below:

src/tasks/cleanup.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_installation/manifest.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_installation/manifest.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_installation/manifest.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_installation/manifest.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_manager.cr Outdated Show resolved Hide resolved
@barmull barmull force-pushed the #2125-create-common-manifest branch 2 times, most recently from 8bdf96c to 552616c Compare September 27, 2024 14:34
@kosstennbl kosstennbl self-requested a review September 30, 2024 12:27
src/tasks/cleanup.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_installation/manifest.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_installation/manifest.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_installation/manifest.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_installation/manifest.cr Show resolved Hide resolved
src/tasks/utils/cnf_installation/manifest.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_installation/manifest.cr Show resolved Hide resolved
src/tasks/utils/cnf_manager.cr Show resolved Hide resolved
src/tasks/utils/cnf_manager.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_manager.cr Show resolved Hide resolved
@barmull barmull force-pushed the #2125-create-common-manifest branch 3 times, most recently from dc84367 to 9713630 Compare October 3, 2024 14:58
src/tasks/utils/cnf_installation/manifest.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_manager.cr Outdated Show resolved Hide resolved
src/tasks/utils/cnf_manager.cr Show resolved Hide resolved
src/tasks/utils/cnf_manager.cr Outdated Show resolved Hide resolved
@barmull barmull force-pushed the #2125-create-common-manifest branch 3 times, most recently from d57ddca to 0a83d77 Compare October 4, 2024 13:34
@kosstennbl
Copy link
Collaborator

Tested, found some mistakes:

common_manifest is not deleted on cnf_cleanup (should be)
doesn't use last helm version in shards (is required for this PR).

@kosstennbl kosstennbl marked this pull request as draft October 7, 2024 09:14
Adapt all usages of config to the new format.
Remove code that was used by old config.
Update all sample and example configs.

Refs: #2135
Signed-off-by: Konstantin Yarovoy <konstantin.yarovoy@tietoevry.com>
@barmull barmull force-pushed the #2125-create-common-manifest branch 3 times, most recently from ba61349 to cebe752 Compare October 7, 2024 16:53
@kosstennbl
Copy link
Collaborator

Requires #2149 and #2162 to be merged to pass all tests and be ready for merging

- group manifests from several CNFs into one
  common_manifests.yaml file
- add functionality in cleanup.cr to remove this file
- depends on PR in helm library:
  cnf-testsuite/helm#4
- common_manifest functionality: used common_manifest
  instead of templates
- remove methods related to templates

Signed-off-by: barmull <barbora.muller@tietoevry.com>
@barmull barmull force-pushed the #2125-create-common-manifest branch from cebe752 to f4021c8 Compare October 8, 2024 12:41
@martin-mat martin-mat self-requested a review October 8, 2024 14:50
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.

4 participants