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

tests/nested/manual/remodel-offline: use the fakestore #14851

Open
wants to merge 1 commit into
base: fde-manager-features
Choose a base branch
from
Open

tests/nested/manual/remodel-offline: use the fakestore #14851

wants to merge 1 commit into from
+315 −29

Conversation

valentindavid
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the Run Nested -auto- Label automatically added in case nested tests need to be executed label Dec 12, 2024
@valentindavid valentindavid added Run nested The PR also runs tests inluded in nested suite FDE Manager Pull requests that target FDE manager branch labels Dec 12, 2024
@valentindavid valentindavid reopened this Dec 12, 2024
@codecov Codecov
Copy link

codecov bot commented Dec 12, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (fde-manager-features@d8bcf84). Learn more about missing BASE report.

Additional details and impacted files 
@@                   Coverage Diff                   @@
##             fde-manager-features   #14851   +/-   ##
=======================================================
  Coverage                        ?   78.20%           
=======================================================
  Files                           ?     1160           
  Lines                           ?   154902           
  Branches                        ?        0           
=======================================================
  Hits                            ?   121147           
  Misses                          ?    26248           
  Partials                        ?     7507
Flag Coverage Δ
unittests 78.20% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@valentindavid valentindavid force-pushed the valentindavid/remodel-offline-fakestore branch 2 times, most recently from e50447f to 36b568e Compare December 12, 2024 16:17
@pedronis pedronis force-pushed the fde-manager-features branch from 5802840 to 5ec2e43 Compare January 13, 2025 09:50
@valentindavid valentindavid force-pushed the valentindavid/remodel-offline-fakestore branch 2 times, most recently from 8100567 to f874793 Compare January 16, 2025 14:31
@valentindavid valentindavid marked this pull request as ready for review January 17, 2025 13:57
@valentindavid valentindavid requested review from andrewphelpsj and alfonsosanchezbeato and removed request for andrewphelpsj January 17, 2025 13:58
@valentindavid valentindavid force-pushed the valentindavid/remodel-offline-fakestore branch 2 times, most recently from 68104b6 to 45639d4 Compare January 20, 2025 12:03
alfonsosanchezbeato
alfonsosanchezbeato reviewed Jan 21, 2025
View reviewed changes
Copy link
Member

@alfonsosanchezbeato alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

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

Nice! This change will also help to extend this test to UC22/24 in the future. Just a couple of minor comments.

tests/nested/manual/remodel-offline/repack-kernel.sh Outdated
Comment on lines 23 to 27
add-apt-repository ppa:snappy-dev/image -y
apt-get install -y golang ubuntu-core-initramfs
Copy link
Member

Choose a reason for hiding this comment

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

Now we build and install ubuntu-core-initramfs as part of the tests in master for UC24+, but I see that the test is for UC20/22. Nevertheless worth adding a comment, saying that for UC24+ we should push the u-c-i package to the container and install instead.

tests/nested/manual/remodel-offline/repack-kernel.sh
Comment on lines +15 to +23
while ! resolvectl query api.launchpad.net; do
waited=$((waited+1))
if [ "${waited}" -gt 120 ]; then
break
fi
sleep 1
done
Copy link
Member

Choose a reason for hiding this comment

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

It is a bit strange that we have to wait here, was this a common failure? Do we have an idea on why this problem happens? Maybe worth adding a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is lxd that does not tell us when the network is ready. This script runs in lxd.

@alfonsosanchezbeato alfonsosanchezbeato mentioned this pull request Jan 21, 2025
Merged
alfonsosanchezbeato
alfonsosanchezbeato approved these changes Jan 22, 2025
View reviewed changes
Copy link
Member

@alfonsosanchezbeato alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

andrewphelpsj
andrewphelpsj approved these changes Jan 22, 2025
View reviewed changes
Copy link
Member

@andrewphelpsj andrewphelpsj left a comment

Choose a reason for hiding this comment

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

Thank you!

@valentindavid valentindavid force-pushed the valentindavid/remodel-offline-fakestore branch from 6285e7f to a006ed9 Compare January 22, 2025 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewphelpsj andrewphelpsj andrewphelpsj approved these changes

@alfonsosanchezbeato alfonsosanchezbeato alfonsosanchezbeato approved these changes

Assignees
No one assigned
Labels
FDE Manager Pull requests that target FDE manager branch Run Nested -auto- Label automatically added in case nested tests need to be executed Run nested The PR also runs tests inluded in nested suite
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@valentindavid @alfonsosanchezbeato @andrewphelpsj