Skip to content

chore: updates CI runners and handles failing Windows test#101

Open
rosygmiki wants to merge 3 commits into
containers:mainfrom
rosygmiki:windows-system-test
Open

chore: updates CI runners and handles failing Windows test#101
rosygmiki wants to merge 3 commits into
containers:mainfrom
rosygmiki:windows-system-test

Conversation

@rosygmiki

@rosygmiki rosygmiki commented May 27, 2026

Copy link
Copy Markdown
Collaborator

This PR temporarily skips an issue where Windows builds fails when source files are missing. It also cleans up our CI workflow by removing unsupported platforms.

Tests

  • Skips the missing source file test on Windows.
  • Includes a bug fix which changes nil to err and adds a new test function for the added error path to satisfy Codecov patch % requirement.

Chores

  • Removes CI workflow support for macOS on Intel and Windows on ARM.
  • Updates go.mod and README badge with Go 1.26.

Closes #84

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request fixes a bug in pkg/tar-patch/apply.go where an error encountered while closing the current file was being ignored and nil was returned instead of the actual error. The code has been corrected to return the err. There are no review comments to address.

@codecov

codecov Bot commented May 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.61%. Comparing base (752790f) to head (9810239).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #101      +/-   ##
==========================================
+ Coverage   81.43%   81.61%   +0.17%     
==========================================
  Files          10       10              
  Lines        1115     1115              
==========================================
+ Hits          908      910       +2     
+ Misses        117      116       -1     
+ Partials       90       89       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rosygmiki rosygmiki changed the title fix: returns error when closing call fails - resolves potential bug fix: validate source files exist before applying patch Jun 2, 2026
@rosygmiki rosygmiki force-pushed the windows-system-test branch 4 times, most recently from b3fbf61 to 27f0279 Compare June 3, 2026 20:14
@rosygmiki rosygmiki force-pushed the windows-system-test branch from cdbe4e8 to 1b77c2a Compare June 12, 2026 13:10
@rosygmiki rosygmiki changed the title fix: validate source files exist before applying patch chore: updates CI runners and handles failing Windows test Jun 12, 2026
Signed-off-by: Rosy-Glorious Miki <rmiki@redhat.com>
…ion for broader compatibility

Signed-off-by: Rosy-Glorious Miki <rmiki@redhat.com>
@rosygmiki rosygmiki force-pushed the windows-system-test branch 2 times, most recently from 4ce93ec to 2fe3ca6 Compare June 12, 2026 15:24
Signed-off-by: Rosy-Glorious Miki <rmiki@redhat.com>
@rosygmiki rosygmiki force-pushed the windows-system-test branch from 2fe3ca6 to 9810239 Compare June 12, 2026 15:47

@djach7 djach7 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A couple of change requests, thanks for working on this!

Comment thread pkg/tar-patch/apply.go

// SetCurrentFile opens the specified file for reading.
func (f *FilesystemDataSource) SetCurrentFile(file string) error {
if f.currentFile != nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Possible change, feel free to ignore if you think this is out of scope for the PR.

I think the change to err here is good, but looking at this harder there's a potential leak in the following scenario:

  • the close fails on line 68
  • we set currentFile to nil on line 69
  • we return the err on line 71 but we now have a file that's still open without a way to reference it because we set the reference to nil

I asked Claude about it and it fired off this suggested fix:

      if f.currentFile != nil {
          err := f.currentFile.Close()
          if err != nil {
              // Don't nil out currentFile if close fails
              return fmt.Errorf("failed to close current file: %w", err)
          }
          f.currentFile = nil
      }
      currentFile, err := os.Open(filepath.Join(f.basePath, file))
      if err != nil {
          return err
      }
      f.currentFile = currentFile
      return nil
  }

Again, I know you didn't make any changes here other than the nil -> err one so if you want to kick this to a later PR or issue that's fine. I just spotted it and wanted to call it out

Comment thread tests/test-tar-errors.sh
# tar-patch: valid delta, missing source file
# Force bsdiff-sized payload + -max-bsdiff-size so the delta emits OPEN for data/only.txt;
# otherwise copyRest-only deltas can apply without that file and expect_fail would flake.
# Skips on Windows platform

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it would be beneficial to add a little more of a "why" to this comment, just a line or two explaining why we're skipping windows for now

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.

System tests for Window compatibility

2 participants