-
Notifications
You must be signed in to change notification settings - Fork 43
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
testutil/pkgdata: Create deb programatically #42
Conversation
2baf145
to
d5164d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is nice, and the logic looks good too, thank you. In the comments below, I think the main theme is that this seems to be doing a bit too much for something that is supposed to be a tiny helper to produce a trivial deb package. There's some extensibility which is unused and untested, and might as well be dropped. As a suggested approach, I'd try to do the minimum required to get a deb manufactured from short inlined content that is just enough for our purposes.
internal/testutil/pkgdata.go
Outdated
content = entry.content | ||
|
||
if content == nil { | ||
sourcePath := entry.source |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just using inlined content all the time, and using a small amount of data instead? We generally don't care about the data, I think. The base-files use was simply a tiny package that allowed us to not worry about it, but if we're going to create a packge from the ground up, we might as well do the minimum amount of work for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm now inlining the content in package definition. However, I still include base-files and read the content via a helper function (readPkgdataFile
), as we have a lot of tests that depend on various files in base-files and their content (via hashes). Changing all the tests for that doesn't sound like minimum amount of work. :-) I propose that from now on, we add new package data as you say, by inlining small amount of data directly, but we keep base-files data. Is that acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested in a different comment, include the new data and new tests, keep the old data and tests as-is, port the old tests to the new inlined data in a separate PR. Let's please not add unwanted logic to avoid unwanted logic, as instead of moving forward that just adds further technical debt to be fixed.
Thanks for detailed review. You are right that it was needlessly complex. The current version is much simpler. Note that there still some magic in |
d5164d1
to
6f101ce
Compare
6f101ce
to
edb6563
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes sense and LGTM. thanks! I only have a couple of comments/questions, and one request: could you please add docstrings? It would make the code easier to follow ;)
internal/testutil/pkgdata.go
Outdated
|
||
type tarEntry struct { | ||
header tar.Header | ||
noFixup bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this actually used for? I don't think is it used in this base-files
test
internal/testutil/pkgdata.go
Outdated
} | ||
|
||
var pkgs = map[string][]tarEntry{ | ||
"base-files": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there a way we could dynamically generate this tarEntry
structure from an existing DEB?
my thinking here is: while I agree it is a must to have this default, hardcoded DEB for testing, it would also be useful if we could somehow dynamically add other DEBs to this map
, at test time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per earlier comment, and reply to your recent comment just now, let's please not introduce all those files in the repository and more logic to this implementation just to then have to remove them. Instead, it's okay to have a new custom package that is tailored specifically for our tests, with small amounts of inlined data. We don't need to port old tests right away.. just add the specific small amount of data you need for your new test to work, and maybe adapting the tests around it as well if that's useful, and then let's port older tests separately.
internal/testutil/pkgdata.go
Outdated
Name: "./bin/", | ||
}, | ||
}, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The standard convention for formatting such lists looks like this:
"base-files": {{
header: tar.Header{
Name: "./bin/",
}, {
Name: "...",
}, {
...
}},
}}
This saves lots of empty spacing that makes it harder to go through the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a side note, I think this PR could help keep the consistency for formatting
4c1e1b7
to
4a9c01b
Compare
4a9c01b
to
cd75f89
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, that's very nice. Just one minor about the testing approach, with an accompanying suggestion:
internal/testutil/pkgdata_test.go
Outdated
}} | ||
|
||
//go:embed testdata/pkgdata.deb | ||
var pkgdataExpectedDeb []byte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having the tests comparing against a pre-packaged binary means anyone testing this logic needs to unpack the deb, performs changes, repack the deb, and include the whole binary here. That's exactly what we need to do nowadays for the actual tests we have, so it feels like just moving the goalpost while preserving the same underlying issue.
How about just using our existing "tree-mapping" testing function to verify the output? This is by the end of the day what the actual tests we have will compare the output against, so it feels realistic enough for our purposes, even more when what we have here is really a belt-and-suspender kind of approach in the sense we're testing the testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@niemeyer I've removed the embedded deb file and now I'm checking tar entries against expected list. I didn't use the tree mapping code, because here, I can just check the list of entries coming out of tar Reader. IOW, I don't need to touch the file system to test this code.
e4be7aa
to
1d3930a
Compare
Currently we only test with base-files deb which is embedded as base64 encoded binary in testutil/pkgdata.go. If we want to change the contents of the data files, we have to un-ar it, un-tar it, change the data files, tar it and ar it. Furthermore, these operations modify attributes based on the workstation on which this process is run. So we end up with arbitrary user/group ids/names names and dates. Fix it by creating debs programatically. The new interface provides way to create debs with arbitrary content of data tarball.
1d3930a
to
a3315e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Currently we only test with base-files deb which is embedded as base64
encoded binary in testutil/pkgdata.go. If we want to change the contents
of the data files, we have to un-ar it, un-tar it, change the data
files, tar it and ar it. Furthermore, these operations modify attributes
based on the workstation on which this process is run. So we end up with
arbitrary user/group ids/names names and dates.
Fix it by creating debs programatically. The new interface provides way
to create debs with arbitrary content of data tarball.