Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
meta: version from git support. #1260
Conversation
chipaca
approved these changes
Apr 18, 2017
I think it's fine, although I recommend tightening up the regexp a little bit.
| + '--always']).decode(sys.getfilesystemencoding()).strip() | ||
| + return '0+git.{}'.format(output) | ||
| + | ||
| + m = re.search(r'(?P<tag>.*)-(?P<revs_ahead>.*)-g(?P<commit>.*)', |
chipaca
Apr 18, 2017
Member
to avoid surprises I'd suggest anchoring this at both ends (i.e. start it with ^, end it with $). Also if revs_ahead is expected to be a number, you want \d+, not .*. Also nothing here should be .*; you want at least one char, and they should all be non-whitespace probably (so \S+), and I suspect commit is expected to be hexadecimal so [0-9a-fA-F]+.
sergiusens
Apr 18, 2017
Collaborator
For the tag, it can be anything but whitespace, so I stole our regex in place from schema/snapcraft.yaml which for version is ^[a-zA-Z0-9.+~-]*$
sergiusens
added some commits
Apr 18, 2017
| + parts: | ||
| + nil: | ||
| + plugin: nil | ||
| + """), file=f) |
elopio
Apr 19, 2017
Member
Almost all the other tests use an existing yaml instead of writing one. Is there a reason here to do it differently?
sergiusens
Apr 19, 2017
Collaborator
Because there are no files aside from snapcraft.yaml added. I've been finding it a pain to easily troubleshoot these split ones when they break.
elopio
requested changes
Apr 19, 2017
A few problems I found testing this:
When I don't have a tag, this message is printed:
fatal: No names found, cannot describe anything.
The error message is not terrible, but it has no context at all. And it says fatal. I would put a warning with more information explaining that this is while trying to set the version, and without the word fatal.
When the snap is not in a git repository, I get this error:
fatal: Not a git repository (or any of the parent directories): .git
fatal: Not a git repository (or any of the parent directories): .git
Command '['git', '-C', '/tmp/local-plugin', 'describe', '--tags', '--dirty', '--always']' returned non-zero exit status 128
That error should explain that the git keyword can only be used when the snapcraft.yaml is in a git repo.
And finally, PR should be accompanied by a pr in snappy-docs explaining the new feature.
| + def generate_version(cls, *, source_dir=None): | ||
| + """Return the latest git tag from PWD or defined source_dir. | ||
| + | ||
| + The output depends on the use of annotated tags, an will return |
| + output = subprocess.check_output([ | ||
| + 'git', '-C', source_dir, 'describe', | ||
| + '--dirty']).decode(sys.getfilesystemencoding()).strip() | ||
| + except subprocess.CalledProcessError: |
elopio
Apr 19, 2017
Member
It would be nice to add a comment here about what does this exception mean.
| + def test_version(self): | ||
| + if self.tag: | ||
| + self.output_mock.return_value = self.return_value.encode('utf-8') | ||
| + else: |
elopio
Apr 19, 2017
Member
I would prefer to move the no-tag case to a different test, to avoid the if block here.
This is what the forum post is for |
sergiusens
added some commits
Apr 19, 2017
is fixed |
this is no longer shown |
|
@sergiusens: I don't think that this is enough: It's not mentioning that the error comes from the version field. My first reaction would be to check the parts, not the version. And as the parts are likely to come from git, I would find myself confused for a while. I'm pretty happy with the rest, and tested it enough. So I'll leave my +1. But please consider extending the message. |
sergiusens
added some commits
Apr 19, 2017
sergiusens
added this to the 2.29 milestone
Apr 19, 2017
sergiusens
merged commit 1a936f4
into
snapcore:master
Apr 19, 2017
1 check passed
sergiusens
deleted the
sergiusens:version-from-git
branch
Apr 19, 2017
added a commit
that referenced
this pull request
Apr 27, 2017
sergiusens commentedApr 17, 2017
If
version: gitis set, snapcraft will determinethe version using git and its tags.
Signed-off-by: Sergio Schvezov sergio.schvezov@canonical.com