cli: add version command #1746

Open
wants to merge 47 commits into
from

Conversation

Projects
None yet
Contributor

gsilvapt commented Nov 20, 2017

  • As discussed in LP #1590349, snapcraft should return the version
    number when called like snapcraft version and snapcraft --version.
  • 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?

+
+ def test_has_version_without_hyphens(self):
+ result = self.run_command(['version'])
+ print(result.output)
@kalikiana

kalikiana Nov 20, 2017

Collaborator

In general, tests shouldn't print anything - instead I would recommend you do something like self.assertThat(result.output, Equals(snapcraft.__version__)) which will only output the value if it fails.

def test_has_version(self):
result = self.run_command(['--version'])
self.assertThat(result.exit_code, Equals(0))
+
+ def test_has_version_without_hyphens(self):
@kyrofa

kyrofa Nov 20, 2017

Member

I'd like to see one more test ensuring that the output from --version == the output from version. Because I can tell you right now, it doesn't 😉 .

Run both --version and version, and you'll see the output is not exactly the same. Click is using its default template, we need to duplicate that here.

@kyrofa kyrofa changed the title from Implemented method to get snapcraft version. to cli: add version command Nov 20, 2017

Contributor

gsilvapt commented Nov 21, 2017

Thank you for your review. Indeed it is important to add that.

But I do not want to hard code the text string before the method returns the version number. Maybe I am missing something from the click framework to also include in the method I wrote? This would actually allow the results to be tested.

Member

kyrofa commented Nov 21, 2017

To quote the click.version_option section in the click docs:

message – custom message to show instead of the default ('%(prog)s, version %(version)s')

I don't have a problem hard-coding that string here. However, if your concern is that Click might change out from under us, you can also add the message parameter where we use version_option to ensure they always stay in sync.

Implemented method to get snapcraft version.
* As discussed in LP #1590349, snapcraft should return the version
number when called like snapcraft version and snapcraft --version.

Rewritting the test to remove unnecessary prints.

Tidying version.py to pass tests.

Refactor new version method

The previous attempt was not printin the same as the --version
command.
On the other hand, to avoid the default Click api being changed
and thus they stop printing the same again, I overrode the
default's method message to "snapcraft, version xxx".

Both methods now print snapcraft, version xxx".

Code cleanup - Remove whitespaces
Contributor

gsilvapt commented Nov 22, 2017

Both methods are now printing the same output and the same text. A test was added to guarantee this.

Collaborator

kalikiana commented Nov 23, 2017

@gsilvapt I see a failure from flake8:
snapcraft/cli/__init__.py:102: error: Unexpected keyword argument "message" for "version_option" /usr/local/lib/mypy/typeshed/third_party/2and3/click/decorators.pyi:174: note: "version_option" defined here

I think you should probably add # type: ignore here, it makes mypy pass - @sergiusens Can you confirm if this the right solution?

Contributor

gsilvapt commented Nov 23, 2017

I can give that a try, for sure. It's awkward that it passed all tests locally 😅

+ def test_method_return_same_value(self):
+ result1 = self.run_command(['version'])
+ result2 = self.run_command(['--version'])
+ self.assertEqual(result1.output, result2.output)
@kyrofa

kyrofa Nov 24, 2017

Member

Perfect, thank you!

Member

kyrofa commented Nov 24, 2017

It's almost like it's validating against its own bundled (and apparently old) version of Click. @sergiusens could use some guidance here.

kyrofa approved these changes Nov 24, 2017

Thanks for this, @gsilvapt! We need to figure out the static failure, but that's not really on you. Overall this PR looks good.

Contributor

gsilvapt commented Nov 24, 2017

Oh, nice! Thank you for reviewing this PR all the time, @kyrofa.
Keep me posted about this issue so I know how to fix if something happen next time.

@kalikiana kalikiana requested a review from sergiusens Dec 4, 2017

snapcraft/cli/version.py
+ snapcraft version
+ snapcraft --version
+ """
+ click.echo('snapcraft, version {}'.format(snapcraft.__version__))
@sergiusens

sergiusens Dec 4, 2017

Collaborator

add a constant here with the version message, make it look like the current on in click, from the source:

'%(prog)s, version %(version)s'
@gsilvapt

gsilvapt Dec 8, 2017

Contributor

I had to hardcode "snapcraft, version %(version)" otherwise it would produce issues.

Thank you for your review. Hopefully I did not mess up commit after a long time (had a couple of busy weeks). If you prefer, I can start over. to avoid multiple commits and such.

snapcraft/cli/__init__.py
@@ -100,7 +99,8 @@ def list_commands(self, ctx):
@click.group(cls=SnapcraftGroup, invoke_without_command=True)
-@click.version_option(version=snapcraft.__version__)
+@click.version_option(message='snapcraft, version %(version)s',
@sergiusens

sergiusens Dec 4, 2017

Collaborator

use the const defined in version

@gsilvapt

gsilvapt Dec 8, 2017

Contributor

this command changed to `( = MESSAGE_SNAPCRAFT_VERSION,version = snapcraft.version)

Hey there, nice job, I only have one request for consistency and book keeping.

Thanks

gsilvapt and others added some commits Dec 8, 2017

Changes version command.
	* Desired output is snapcraft, version x.yz
elf: conversion from libraries (#1744)
Converting the libraries module into elf and making it more focused on handling elf
files as a whole.

Fixes: #1666
Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
kernel plugin: respect the kernel-with-firmware property (#1750)
Do not ignore the `kernel-with-firmware` property and assume `./lib/firmware` is present

Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com>
tests: add a script to run autopkgtests from the ppa (#1749)
Use the snapcraft built in the ppa as the subject of adt.
nodejs plugin: pass proxy configuration to yarn (#1754)
yarn apparently doesn't pick this up from the environment, so we need to
tell it explicitly.

LP: #1733718
Signed-off-by: Colin Watson <cjwatson@ubuntu.com>
many: ensure classic confined binaries use the correct interpreter (#…
…1759)

During the prime step, the pluginhandler collects all elf files,
as a postprocessing step, the elf files that contain an .interp
section in the elf headers are set to use the linker loader
from core.

Fixes: #1662

Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
store: push-metadata added. (#1634)
Pushes metadata to the store and has logic to help resolve conflicts.
python plugin: use finer grained file filters for pip (#1763)
This allows Python scripts that begin with "pip" but aren't actually pip
(such as pip2pi and friends) to be included in snaps using the Python
part.

LP: #1734213
tests: push metadata test needs to push the snap first (#1766)
Enhanced fake store to detect that metadata cannot be pushed prior to pushing 
the binary, and fixed the test.
tests: move the catkin integration tests to a separate suite (#1760)
Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
lxd: let lxd choose the architecture (#1718)
lxd implements exactly the same logic as snapcraft does to choose an
architecture when one is not specified, so remove snapcraft's copy.

It is still necessary to record the architecture of the created
container to detect whether snaps can just be copied across, but this
is now done after the container is started, which will still be correct
when/if support for building on a non-default image is merged.
store: refactor acquirement of attenuated macaroon (#1765)
This paves the way toward using it outside of just `enable-ci travis`.

Resolve #1709

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
project: export the arch triplet into the environment (#1768)
It is exported as SNAPCRAFT_ARCH_TRIPLET and usable by
the scripting functionality in snapcraft.

Fixes: #1752

Signed-off-by: Ubuntu <sergio.schvezov@canonical.com>
snapcraft.yaml: use gcc to determine tuple (#1764)
Currently, when building the Snapcraft snap, we try to create the
libsodium.so symlink by manually constructing the arch tuple using
`uname`. However, this does not produce the correct tuple in all cases,
particularly i686.

Use `gcc -print-multiarch` instead. It's exactly what we need, and it's
already a `build-package`.

LP: #1733922

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
spelling: fixed spelling and added codespell support (#1770)
If applied, this commit will fix all spelling errors in the current
state of the repository. Furthermore, add a test in .travis.yaml to do
so on pull requests.

Based on Google Code-in task
https://codein.withgoogle.com/dashboard/task-instances/5347598271512576/
tests: add --help command to the runtests.sh script (#1775)
LP: #1702716
Signed-off-by: Marcin Mikołajczak <me@m4sk.in>
tests: run codespell as part of static tests (#1783)
Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
cli: show helpful message for 'snapcraft list-keys' with no keys (#1784)
Tell user to see 'register-key --help' if there is no key registered. Before this fix, an empty table was printed. (which is ugly and redundant)

LP: #1720210
Signed-off-by: BayMinimum <bearksb1115@gmail.com>
static tests: add type hinting to file_utils.py (#1785)
Add type hinting support to snapcraft/file_utils.py according to PEP 484
catkin-tools plugin: use stage-packages (#1779)
Currently the catkin-tools plugin uses `build-packages` for its build
dependency instead of `stage-packages`, which fails on clean systems.

Use `stage-packages` instead. Also, since this would have been caught by
an integration test, add one of those as well (to the snapd suite,
specifically).

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
tests: python plugin integration tests moved to a separate suite (#1791)
The plugins suite is again causing time outs because it's too big.
We discussed about an ugly hack to split the suite, and we already applied it to the catkin plugin. Here we are doing the same for python, and we might have to do it again for ruby or catkin.
storeapi: support for pushing binary metadata (#1774)
Update the push-metadata command to also push binary metadata (so far, icon only).
cli: improve the help command (#1789)
Give it parity with --help but also extend it so topics and plugin help is
not completely hidden.

LP: #1698116
Fixes: #1698

Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
cli: add export-login command (#1780)
Also add a `--with` option to `login` to use the exported login file.

Resolve #1711.

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
many: set rpath for elf files for classic (#1781)
* many: set rpath for elf files for classic

When working with classic confinement `LD_LIBRARY_PATH` cannot be set without consequences
on when other assets on the host are called from the snap.

This sets an rpath for every elf file when confinement is set to classic
and allows for further extending this mechanism to non classic.

When setting rpaths it also makes use of $ORIGIN to setup relative paths.

Fixes: #1663
Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
tests: update the snap name already registered for store tests (#1797)
The previous snap name we were using for this test was revoked at some point.
The store is now returning a different message for revoked snaps, so we needed
to refresh this sceneario with a new name registered by someone else, but not
revoked.
Changes version command to a more update state.
	Uses variables to produce the output instead of hard-coding the desired text.

Changes --version command to suit click handler.
cli: Modifies version command.
	* Using variable %(prog) will produce errors in the tests.
	* Replaces %(prog) with string literal "snapcraft".

@sergiusens sergiusens added this to the 2.38 milestone Dec 14, 2017

Contributor

gsilvapt commented Dec 14, 2017

There is unnecessary white space causing the static tests to fail. If I fix this, some conflicts arise in other files. I need guidance here, if you please. I can push a single commit just to fix the white spaces. I wanted to squash this into the previous commit but it is a no-brainer for me.

@sergiusens sergiusens modified the milestones: 2.38, 2.39 Dec 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment