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

recording: record the packages installed in the machine #1529

Merged
merged 1 commit into from
Sep 20, 2017

Conversation

come-maiz
Copy link
Contributor

@come-maiz come-maiz commented Sep 4, 2017

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • If this is a bugfix. Have you checked that there is a bug report open for the issue you are trying to fix on bug reports?
  • If this is a new feature. Have you discussed the design on the forum?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh unit?

This closes #1452.
Requires #1528.


with apt.Cache() as apt_cache:
expected_package = 'python3={}'.format(
apt_cache['python3'].installed.version)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about checking repo.get_installed_packages as well? My gut feeling is we should also explicitly test that it works correctly (even if the manifest uses it indirectly) - or if not here, in a unit test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a unit test in snapcraft/tests/repo/test_deb.py

@@ -754,16 +753,20 @@ def setUp(self):
'internal', 'repo', 'manifest.txt'))) as manifest_file:
self.add_packages([line.strip() for line in manifest_file])

def add_package(self, package):
package.temp_dir = self.path
self.cache[package.name] = package
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you create the FakeaptCachePackage here instead of every call to the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't want the add_package method to receive all the arguments that FakeAptCachePackage. If you want to just add a package by name you can self.add_packages([test]). If you need more control than the name, I think it's ok that you instantiate your own.

Maybe that other method should be called add_packages_by_name.

@sergiusens sergiusens added this to the 2.35 milestone Sep 11, 2017
@come-maiz come-maiz force-pushed the installed_packages branch 3 times, most recently from b4f1ccc to b3b1a1d Compare September 15, 2017 04:56
@sergiusens
Copy link
Collaborator

@ElOpio what is the last part of #1452? Doesn't this fix it completely?

@sergiusens sergiusens merged commit d0591dd into canonical:master Sep 20, 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
Development

Successfully merging this pull request may close these issues.

3 participants