tests: Include the tools from snapd-testing-tools project in "$TESTSTOOLS" #10825
Conversation
A new submodule is included in "$TESTSLIB"/external, the idea is to have a single directory for tools, so all the tools are copied to the $TESTSTOOLS directory Initially are not deleted the tools, this will happen in a following change
Codecov Report
@@ Coverage Diff @@
## master #10825 +/- ##
=======================================
Coverage 78.19% 78.20%
=======================================
Files 914 914
Lines 103549 103549
=======================================
+ Hits 80968 80976 +8
+ Misses 17498 17493 -5
+ Partials 5083 5080 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Are there other alternatives to using a git submodule? Do we know how well other environments like the autopkgtest and whatever the Fedora equivalent is work with git submodules? I have found git submodules to often be poorly integrated in these environments leading to confusion and requiring fixes to get them to "just work".
|
Indeed somehow other PR's not touching this branch are somehow broken due to the existence of submodules, thus confirming my suspicion that submodules break everything they touch 😢 |
|
This branch broke our self-hosted runners, see actions/checkout#590, let's close this in the meantime so we can try to fix our self-hosted runners |
|
@anonymouse64 So I could validate that it shouldn't affect autopackage tests due we can remove the submodule configuration in case we need that before sync with launchpad in https://github.com/snapcore/spread-cron/blob/master/branches/snapd-vendor-sync/target/tasks/snapd-vendor-sync/task.yaml |
branch = main
The test is maintained because we need to check core systems here
There was a problem hiding this comment.
This looks fine, thanks for working on this. My only concern/wondering is if we could try a "git subtree" instead, I don't have much experience with either but I heard a lot of bad stories about usbmodules and people seems to be more happy with subtrees but I could be wrong here, happy to get others opinions.
.github/workflows/test.yaml
Outdated
| @@ -282,6 +282,7 @@ jobs: | |||
| with: | |||
| # spread uses tags as delta reference | |||
| fetch-depth: 0 | |||
| submodules: true | |||
There was a problem hiding this comment.
I wonder if a git subtree might be an option?
There was a problem hiding this comment.
@mvo, thanks for the review, I already tried git subtree, both work well. Let me move to subtree and we can decide which is a better solution. Something that I faced last week and I didn't like it is that to use submodules you need git 2.18, and that could be a problem to clone the repo in an old distro.
There was a problem hiding this comment.
@mvo it is already updated using git subtree
There was a problem hiding this comment.
We need to remember that we need to pull changes from snapd-testing-tools project, but it has to be done as part of a PR I suppose.
There was a problem hiding this comment.
@anonymouse64 @mvo5 please tell me what do you prefer, submodules or subtree?
There was a problem hiding this comment.
TBH, I've never used subtree before, but it seems like a perfectly acceptable option so long as we don't create commits which span both the subtree and the main project. Let's try it.
…t 020f432898 git-subtree-dir: tests/lib/external/snapd-testing-tools git-subtree-split: 020f43289802d78d90db3571ad3ee281bce43246
…/external/snapd-testing-tools'
This is because shellcheck is getting stuck running shellcheck when using a git subtree. Shellcheck is being executed in the project snapd-testing-tools.
…t 638ddd7d23 git-subtree-dir: tests/lib/external/snapd-testing-tools git-subtree-split: 638ddd7d23e13bbff4fb22f523c999954bd8b4d8
…/external/snapd-testing-tools'
|
This looks nice, thanks you! One thing maybe worth adding is how to update/list/keep-track of the subtree in a readme file, maybe as part of the test-tools themselfs? |
|
Also is it possible to squash-merge this? The history is now a bit noisy :) |
|
@mvo5 yes, it is ready to squash-merge, the error are unrelated. |
|
I'm not sure if we can squash merge this, I have read that you shouldn't have commits which touch both the subtree and the main project at once because then importing new commits from the subtree repo will get confused with a different history to them and you will have to sort out those merges. So probably the thing to do is to make sure that the file contents of the subtree is exactly the same as master tip in the other repo, then rebase this PR (or create a new one like this if that's easier) with two commits, one adding the subtree, and the other commit fixing up the rest of the snapd repo to operate using the subtree instead. |
There was a problem hiding this comment.
one super minor nitpick, but note that we should be careful about squash merging here and also I think we should have something in the repo somewhere that says "don't mix commits modifying the subtree with the main tree" or some such. Maybe we could even have something in run-checks / github actions which ensures that doesn't happen
also see my comment about how to proceed on merging this PR
| def is_file_in_dirs(file, dirs): | ||
| for dir in dirs: | ||
| if os.path.abspath(file).startswith('{}/'.format(os.path.abspath(dir))): | ||
| print('Skipping {}'.format(file)) |
There was a problem hiding this comment.
nitpick probably either the name of the function should change or the print message should be outside of this function since the name of the function suggests it is a generic function, but it has this print message in it which makes it less generic
A new submodule is included in "$TESTSLIB"/external, the idea is to have
a single directory for tools, so all the tools are copied to the
$TESTSTOOLS directory
Initially are not deleted the tools, this will happen in a following
change