Fix: export_published_chart correctly detects and exports tar file #2052

svteb · 2024-05-23T08:56:13Z

Description

Removed file globbing as it caused errors and added a new function that will detect a specific tgz file to be untared. Removed the line: FileUtils.rm_rf(tgz_name) as it did not work previously and now that globbing has been fixed it would cause undesired behavior (honestly I was not able to figure out why it is there at all).

Issues:

Refs: #1947

How has this been tested:

Types of changes:

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update

Checklist:

Documentation

My change requires a change to the documentation.
I have updated the documentation accordingly.
No updates required.

Code Review

Does the test handle fatal exceptions, ie. rescue block

Issue

Tasks in issue are checked off

Refs: cnti-testcatalog#1947 - Fixes the issue where multiple tar files caused the Tar module to fail due to globbing, by selecting a single tgz file. - There was a line that was supposed to delete the tgz file (no idea why): FileUtils.rm_rf(tgz_name), which did not actually work due to the previously mentioned globbing, it was removed because it would now cause the program to fail. Signed-off-by: svteb <[email protected]>

src/tasks/utils/cnf_manager.cr

Refs: cnti-testcatalog#1947 - Backtracked on removal of: FileUtils.rm_rf(tgz_name), made it functional through Dir.glob("#{Helm.chart_name(helm_chart)}-*.tgz"). Through this addition a state where multiple tgz archives are present is not possible. - Also added an exception in case a prior helm failure happens and no archive is pulled (not sure whether this state can be reached). Signed-off-by: svteb <[email protected]>

svteb · 2024-05-28T09:28:51Z

Comments from @martin-mat were correct in pointing out some unhandled cases/lack of information provided. I've come to understand the intentions behind FileUtils.rm_rf(tgz_name) and decided to bring it back:

unless input_file && !input_file.empty?
      files_to_delete = Dir.glob("#{Helm.chart_name(helm_chart)}-*.tgz") ~> addition
      files_to_delete.each do |file|
        FileUtils.rm(file)
        puts "Deleted: #{file}"
      end ~> end of addition

      helm_info = Helm.pull(helm_chart) 
      unless helm_info[:status].success?
        puts "Helm pull error".colorize(:red)
        raise "Helm pull error"
      end
end

It was necessary to do it like this because FileUtils.rm_rf does not work with file globbing which was originally present. This addition also removed the necessity of checking for state where multiple versions of some helm archive were present (as they get deleted and the newest one gets pulled subsequentially).

Another mistake from the previous commit, that was undiscovered, was the necessity of moving this entire block before the block where tgz archive is being discovered, as the Helm.pull(helm_chart) command is what downloads the tgz archive. This also makes the sequence of steps somewhat more logical:

helm_pull -> discover_tgz -> extract_tgz whereas the previous sequence was discover_tgz -> helm_pull -> extract_tgz.

Furthermore I added an exception that prints better information in case no tgz file is discovered.

src/tasks/utils/cnf_manager.cr

- Dir.glob functionality for getting tgz files in the working directory abstracted into a function - get_tgz_name function made more generic - outputs made more explicit Signed-off-by: svteb <[email protected]>

src/tasks/utils/cnf_manager.cr

martin-mat

lgtm, good refactoring!

kosstennbl · 2024-07-02T14:48:56Z

src/tasks/utils/cnf_manager.cr

    unless input_file && !input_file.empty?
-      FileUtils.rm_rf(tgz_name)
+      files_to_delete = find_tgz_files(helm_chart)
+      files_to_delete.each do |file|
+        FileUtils.rm(file)
+        Log.info { "Deleted: #{file}" }
+      end
+
      helm_info = Helm.pull(helm_chart) 
      unless helm_info[:status].success?
        puts "Helm pull error".colorize(:red)
        raise "Helm pull error"
      end
    end

-    TarClient.untar(tgz_name,  "#{destination_cnf_dir}/exported_chart")
+    # get the tgz file name
+    if input_file && !input_file.empty?
+      # todo add generate and set tar as well
+      config = CNFManager::Config.parse_config_yml(CNFManager.ensure_cnf_testsuite_yml_path(config_file), airgapped: true) 
+      tar_info = AirGap.tar_info_by_config_src(helm_chart)
+      tgz_name = tar_info[:tar_name]
+    else
+      config = CNFManager::Config.parse_config_yml(CNFManager.ensure_cnf_testsuite_yml_path(config_file), generate_tar_mode: output_file && !output_file.empty?) 
+      tgz_name = get_and_verify_tgz_name(helm_chart)
+    end


I think that these "if" statements should be joined and each logical branch marked with comments:

if input_file && !input_file.empty? # Input file present, existing Tar exctraction mode ... else # Input file absent, Pulled chart tar extraction mode ... end

Great catch, I reduced it to one if else block, should be alright now.

kosstennbl

Other than mentioned problem - looks nice: fixes the Tar problem and refactors the code

Signed-off-by: svteb <[email protected]>

kosstennbl

lgtm

svteb requested review from denverwilliams, wvwatson and agentpoyo as code owners May 23, 2024 08:56

martin-mat requested changes May 24, 2024

View reviewed changes

src/tasks/utils/cnf_manager.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

martin-mat requested changes Jun 3, 2024

View reviewed changes

src/tasks/utils/cnf_manager.cr Show resolved Hide resolved

src/tasks/utils/cnf_manager.cr Outdated Show resolved Hide resolved

Fix: minor refactor of tgz functions and clarified outputs

31f59a9

- Dir.glob functionality for getting tgz files in the working directory abstracted into a function - get_tgz_name function made more generic - outputs made more explicit Signed-off-by: svteb <[email protected]>

martin-mat requested changes Jun 20, 2024

View reviewed changes

src/tasks/utils/cnf_manager.cr Outdated Show resolved Hide resolved

src/tasks/utils/cnf_manager.cr Outdated Show resolved Hide resolved

martin-mat requested changes Jun 25, 2024

View reviewed changes

svteb force-pushed the export_published_chart branch from 2bfb97c to 255da4d Compare July 2, 2024 07:24

martin-mat requested review from taylor, electrocucaracha, rich-l, Smitholi67, kosstennbl, barmull and martin-mat July 2, 2024 13:37

martin-mat approved these changes Jul 2, 2024

View reviewed changes

kosstennbl reviewed Jul 2, 2024

View reviewed changes

svteb force-pushed the export_published_chart branch 2 times, most recently from cdcf9b1 to 4746e73 Compare July 3, 2024 08:32

Fix: Minor abstraction addition and use of warn log instead of info

c764af9

Signed-off-by: svteb <[email protected]>

svteb force-pushed the export_published_chart branch from 4746e73 to c764af9 Compare July 3, 2024 08:34

martin-mat requested a review from kosstennbl July 3, 2024 20:47

kosstennbl approved these changes Jul 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: export_published_chart correctly detects and exports tar file #2052

Fix: export_published_chart correctly detects and exports tar file #2052

svteb commented May 23, 2024 •

edited

Loading

svteb commented May 28, 2024

martin-mat left a comment

kosstennbl Jul 2, 2024 •

edited

Loading

svteb Jul 3, 2024

kosstennbl left a comment

kosstennbl left a comment

Fix: export_published_chart correctly detects and exports tar file #2052

Are you sure you want to change the base?

Fix: export_published_chart correctly detects and exports tar file #2052

Conversation

svteb commented May 23, 2024 • edited Loading

Description

Issues:

How has this been tested:

Types of changes:

Checklist:

svteb commented May 28, 2024

martin-mat left a comment

Choose a reason for hiding this comment

kosstennbl Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

svteb Jul 3, 2024

Choose a reason for hiding this comment

kosstennbl left a comment

Choose a reason for hiding this comment

kosstennbl left a comment

Choose a reason for hiding this comment

svteb commented May 23, 2024 •

edited

Loading

kosstennbl Jul 2, 2024 •

edited

Loading