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

Added test_version parameter to cli #394

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

abdullahzen
Copy link

@abdullahzen abdullahzen commented Dec 26, 2020

Changes made:

  • Added test_version parameter to cli when used with build --test to produce the test artifact with a desired numbered version rather than using the commit_count which is always 0.
  • Kept the default value for the "test_version" as 0.

Fixes #332

@xsuchy
Copy link
Member

xsuchy commented Dec 26, 2020

-1 from me.

Commit_count is the number of commits since the last tagged version. When your workflow is git-commit/tito-build/rpm -U then commit_count assures that package is always upgradable (every subsequent release is bigger than the previous one). Although this breaks when you git commit --amend -a

@abdullahzen
Copy link
Author

@xsuchy
Thank you for taking the time. I do agree with you but I was adding a feature as requested in the issue #332 where he mentions a use case for overriding the version.
I do see where I went wrong tho. I should set the default to commit_count and if test_version is passed, it'll override the commit_count. Thank you for clarifying that for me 👍
Do you believe that would be a good idea to allow both ways and let the test_version only override the count if test_version is set?

version = "git-%s.%s" % (self.commit_count, latest_commit[:7])
if self.test_version is None:
self.test_version = get_commit_count(self.build_tag, latest_commit)
version = "git-%s.%s" % (self.test_version, latest_commit[:7])
Copy link
Member

Choose a reason for hiding this comment

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

+1 LGTM now.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you!

@abdullahzen abdullahzen marked this pull request as ready for review December 29, 2020 05:35
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.

Add option to set build number for --test
2 participants