Skip to content

refactor: filter created entries for report - #124

Merged
niemeyer merged 22 commits into
canonical:mainfrom
letFunny:chisel-db-report-filter
Apr 1, 2024
Merged

niemeyer merged 22 commits into
canonical:mainfrom
letFunny:chisel-db-report-filter

Conversation

@letFunny

@letFunny letFunny commented Mar 5, 2024 •

Copy link
Copy Markdown
Collaborator
  • Have you signed the CLA?

The entries not mentioned explicitly in the slice definition will not be
added to the report (e.g. copyright, parent directories). Additionally,
we have removed the Globbed option and simplified it.

This PR depends on #113.

@letFunny
letFunny requested a review from rebornplusplus March 5, 2024 10:09
@letFunny
letFunny force-pushed the chisel-db-report-filter branch from 2e39b33 to b2d22d9 Compare March 6, 2024 10:51

@rebornplusplus rebornplusplus 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.

I left a few comments below. But overall, it looks okay to me.

Note that I only reviewed 622e346 and b2d22d9 as those are the new ones compared to #113.

Comment thread internal/deb/extract.go Resolved
Comment thread internal/slicer/slicer.go Resolved
Comment thread internal/slicer/slicer.go Resolved
Comment thread internal/slicer/slicer.go Resolved
Comment thread internal/fsutil/create.go
Path: o.Path,
Mode: o.Mode,
Hash: hex.EncodeToString(rp.h.Sum(nil)),
Hash: hash,

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is just a cosmetic change so the hash of folders is an empty string.

"/dir/text-file-3": "file 0644 5b41362b {test-package_myslice}",
},
}, {
summary: "Copyright is installed",

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I had to remove the copyright and move it to a separate test because testing several packages was getting too complex and magical.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds good, thanks for doing that.

@letFunny
letFunny requested a review from niemeyer March 8, 2024 09:31

@niemeyer niemeyer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is looking great, Alberto, thank you.

A few minor details only.

Comment thread internal/deb/extract.go Resolved
Comment thread internal/deb/extract.go Resolved
Comment thread internal/slicer/report.go Resolved
Comment thread internal/slicer/report_test.go Resolved
Comment thread internal/slicer/report_test.go Resolved
Comment thread internal/slicer/slicer.go Resolved
Comment thread internal/slicer/slicer_test.go Resolved
"/dir/text-file-3": "file 0644 5b41362b {test-package_myslice}",
},
}, {
summary: "Copyright is installed",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds good, thanks for doing that.

Comment thread internal/slicer/slicer_test.go Resolved
@letFunny letFunny mentioned this pull request Mar 12, 2024
1 task done
@cjdcordeiro
cjdcordeiro requested a review from niemeyer March 14, 2024 10:05
@cjdcordeiro cjdcordeiro added the Priority Look at me first label Mar 14, 2024

@niemeyer niemeyer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks again for the changes.

@niemeyer
niemeyer merged commit 39ba448 into canonical:main Apr 1, 2024
@niemeyer niemeyer changed the title feat: filter created entries for report refactor: filter created entries for report Apr 1, 2024
@letFunny
letFunny deleted the chisel-db-report-filter branch October 17, 2024 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Priority Look at me first

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants