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.

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

tests/main: use pkgdb function in more test cases #3455

Merged
merged 13 commits into from Jun 19, 2017

Conversation

morphis
Copy link
Contributor

@morphis morphis commented Jun 9, 2017

No description provided.

Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

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

+1 but two questions asked.

@@ -29,8 +31,7 @@ execute: |

. "$TESTSLIB/pkgdb.sh"
echo "Ensure core is gone and we have ubuntu-core instead"
dpkg --purge --force-depends snapd
apt-get install -f -y
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I wonder what this thing is about, why did we apt-get install ... nothing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

apt-get install -f -y actually does dependency cleanup if you did a purge via dpkg --purge

echo "Given openvswitch is installed"
apt install -y --no-install-recommends openvswitch-switch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the --no-install-recommends relevant? I wonder why it was here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I actually added to support for that to the distro_install_package command but looks like this was dropped during the rebase/master I did

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed that. distro_install_package now accepts --no-install-recommends as argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to be more generic: --no-weak-deps, which for Debian could be --no-install-recommends. For openSUSE, it would be --no-recommends, and for Fedora, it'd be --setopt=install_weak_deps=False

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me add these thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

;;
fedora-*)
dnf -q -y install -y "$DNF_FLAGS" $package_name
dnf -q -y install "$DNF_FLAGS" -y $package_name
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you doing -y twice? It's already being used as a global flag...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're faster than me, already about to push another change to fix that.

;;
opensuse-*)
zypper -q install -y "$ZYPPER_FLAGS" $package_name
zypper -q install "$ZYPPER_FLAGS" $package_name
Copy link
Contributor

Choose a reason for hiding this comment

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

You still need -y after install subcommand for Zypper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Narf. Never do multiple things at the same time :-)

@codecov-io
Copy link

codecov-io commented Jun 12, 2017

Codecov Report

Merging #3455 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3455      +/-   ##
==========================================
+ Coverage   77.15%   77.18%   +0.02%     
==========================================
  Files         373      375       +2     
  Lines       25793    25837      +44     
==========================================
+ Hits        19901    19941      +40     
- Misses       4134     4136       +2     
- Partials     1758     1760       +2
Impacted Files Coverage Δ
dirs/dirs.go 96.77% <0%> (-0.06%) ⬇️
daemon/api.go 73.18% <0%> (-0.01%) ⬇️
interfaces/builtin/content.go 80.73% <0%> (ø) ⬆️
cmd/snap/cmd_run.go 64.51% <0%> (ø) ⬆️
interfaces/policy/basedeclaration.go 40.74% <0%> (ø) ⬆️
interfaces/builtin/udisks2.go 82.5% <0%> (ø) ⬆️
cmd/snap/cmd_get_base_declaration.go 75% <0%> (ø)
interfaces/builtin/greengrass_support.go 100% <0%> (ø)
overlord/snapstate/snapstate.go 81.52% <0%> (+0.23%) ⬆️
asserts/repair.go 95.55% <0%> (+1.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8064fa0...0a690c7. Read the comment docs.

Copy link
Contributor

@fgimenez fgimenez left a comment

Choose a reason for hiding this comment

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

LGTM

@fgimenez
Copy link
Contributor

There's an issue with the management of --no-install-recommends https://travis-ci.org/snapcore/snapd/builds/242007338#L1944

@morphis
Copy link
Contributor Author

morphis commented Jun 13, 2017

@fgimenez Thanks, yeah I saw that already. Has something to do with the quoting of APT_FLAGS, not yet sure what it is.

@mvo5 mvo5 merged commit 70180a0 into snapcore:master Jun 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants