Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix custom repo test #415

Merged
merged 3 commits into from
Oct 30, 2024
Merged

Fix custom repo test #415

merged 3 commits into from
Oct 30, 2024

Conversation

cpuguy83
Copy link
Member

@cpuguy83 cpuguy83 commented Oct 24, 2024 •
edited
Loading

A few fixes here:

  1. Minor refactor on custom repo tests so that the proper *testing.T is being used. This fixes a problem with the test build not producing any logs making it hard to figure out what the error is.
  2. Namespace dep package names per test so that we don't have 2 parallel tests using the same package name, potentially creating a race where the package is cached and re-used in the other test.
  3. Remove cached repo data for local repos (e.g., ones created in /top/repo) to fix the case where a repo can appear to become unsigned causing apt to error out.

@cpuguy83 cpuguy83 requested a review from a team as a code owner October 24, 2024 22:22
@cpuguy83 cpuguy83 force-pushed the fix_custom_repo_test branch from 875f2a7 to f5d8334 Compare October 24, 2024 22:22
This was referenced Oct 24, 2024
Merged
Merged
@cpuguy83 cpuguy83 force-pushed the fix_custom_repo_test branch 2 times, most recently from 0ab2bd1 to be765cb Compare October 24, 2024 23:40
@cpuguy83 cpuguy83 marked this pull request as draft October 24, 2024 23:51
@cpuguy83
Fix custom repo test
cd98fc4 
The main fix here is that the test was using the same package
name/version for both test cases.
Due to this, the package being tested may already be in the apt package
cache when the negative test runs.
This can cause unexpected results.

Also some minor refactoring so that the proper `*testing.T` is used.
This fixes a problem with the test build not producing any logs.

Signed-off-by: Brian Goff <[email protected]>
@cpuguy83 cpuguy83 force-pushed the fix_custom_repo_test branch 8 times, most recently from c39e8f6 to f83e36f Compare October 26, 2024 00:46
@cpuguy83
Copy link
Member Author

Ok, so the first reason the test was failing is the cached repo info in /var/lib/apt/lists for the local repo (at /opt/repo).
Particularly problematic due to using the same repo path for all the repo tests and the repo gets flipped from signed to unsigned and apt does not like that.

In general I think we don't want to keep any data about local repo mounts between builds, so fixed this by clearing any local repo data in that dir (local repos apparently start with _ so rm -f /var/lib/apt/lists/_* did the trick.

Now it looks like the custom repo just isn't working for some reason when doing the installation of runtime dependencies. Looking into this.

Depending on the ordering of tests this could pass but usually fails in CI. Locally it's harder to make it fail but it does happen.

@cpuguy83 cpuguy83 force-pushed the fix_custom_repo_test branch from f83e36f to 3268a83 Compare October 26, 2024 20:16
@cpuguy83 cpuguy83 added this to the v0.10.0 milestone Oct 26, 2024
@cpuguy83 cpuguy83 force-pushed the fix_custom_repo_test branch 2 times, most recently from edfb67b to 3b3ae9c Compare October 26, 2024 20:38
@cpuguy83
jammy: Remove cached data for local repos before install
e8c3269 
We don't really need to cache these since they are locally anyway.
It can also cause inconsistencies between builds if the repo config is
different, for instance during tests when swithcing between signed and
unsigned repos.

Signed-off-by: Brian Goff <[email protected]>
@cpuguy83 cpuguy83 force-pushed the fix_custom_repo_test branch from 3b3ae9c to e8c3269 Compare October 26, 2024 20:57
@cpuguy83 cpuguy83 marked this pull request as ready for review October 26, 2024 21:10
@cpuguy83 cpuguy83 requested a review from adamperlin October 26, 2024 21:10
@cpuguy83
Copy link
Member Author

Ok, realized the extra failure was due to the same exact cause, there's just 2 places we are installing packages from and the fix needed to be applied in the 2nd case.

@cpuguy83 cpuguy83 force-pushed the fix_custom_repo_test branch from 781f1b2 to 31cee31 Compare October 27, 2024 17:30
@cpuguy83 cpuguy83 requested a review from pmengelbert October 28, 2024 16:14
adamperlin
adamperlin reviewed Oct 30, 2024
View reviewed changes
frontend/jammy/handle_deb.go
@@ -285,7 +317,8 @@ func buildDepends(worker llb.State, sOpt dalec.SourceOpts, spec *dalec.Spec, tar
)

return in.Run(
installWithConstraints(debPath+"/*.deb", depsSpec.Name, customRepoOpts),
installWithConstraints(debPath+"/*.deb", depsSpec.Name, opts...),
customRepoOpts,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for cleaning this up and addressing your review suggestion from before to add the customRepoOpts instead of threading through install...

@adamperlin adamperlin self-requested a review October 30, 2024 17:07
adamperlin
adamperlin approved these changes Oct 30, 2024
View reviewed changes
Copy link
Contributor

@adamperlin adamperlin left a comment

Choose a reason for hiding this comment

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

I think this looks good to me, and the fixes seems very straightforward. Thank you for fixing these issues!

@cpuguy83 cpuguy83 merged commit a877b9d into Azure:main Oct 30, 2024
9 checks passed
@cpuguy83 cpuguy83 deleted the fix_custom_repo_test branch October 30, 2024 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamperlin adamperlin adamperlin approved these changes

@pmengelbert pmengelbert Awaiting requested review from pmengelbert

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.10.0
Development

Successfully merging this pull request may close these issues.
2 participants
@cpuguy83 @adamperlin