-
Notifications
You must be signed in to change notification settings - Fork 681
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
consolidate version numbers throughout all interfaces #592
Conversation
@@ -7,7 +7,7 @@ var fsextra = require("fs-extra"); | |||
var path = require("path"); | |||
var Helper = require("../helper"); | |||
|
|||
program.version(pkg.version, "-v, --version"); | |||
program.version(Helper.getVersion(), "-v, --version"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember getting the same feeling when implementing the UI version. This is what should be displayed:
- If this is a git repository, git is available and git command is available, display git commit.
- Otherwise, display npm version
As an amendment to (1.), if the commit hash has an exact match for an npm version (i.e. a tag), we may want to display npm version instead. Though it might be a bit confusing as, essentially, instance is running from source and not from a build... so I'd say, if git repo + git available, go for git commit.
However, that npm version + commit hash (like in your picture above) is a no-no for me. As a user, what does that mean, am I running the pre.6 version or a specific commit? Is that commit hash related to that version? If I notice a bug, does that mean pre.6 have to be incriminated?
I'm in favor of simplicity here: an npm version, or a commit hash, but not both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by npm version? All versions are (so far) given git tags as well (which is what git-describe uses to produce these refs).
As a user, what does that mean, am I running the pre.6 version or a specific commit? Is that commit hash related to that version?
I can agree on that. What about 2.0.0-pre.6-31-gbdd3fec (dirty)
. The entire point of this PR is to move from just providing commit SHAs to something with more face value. What about the version displayed on the client? Does this apply to the client as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by npm version?
Sorry, I should have defined terminology I was using:
- What I call "npm version" is that human-friendly "1.5.0", "2.0.0-pre.6" string, whether it comes from a git tag or from the
package.json
file (let's call that simply a "version" from now on) - What I call "commit hash" is... well, a (7-char) git commit hash :-)
The entire point of this PR is to move from just providing commit SHAs to something with more face value.
But that's the issue here. There is no face value at all on an instance that runs a given version + N commits on top of it. We cannot inform the user about a version if a single commit has been added to it. What if that commit reverts all or part of a version (think issues added in pre.6)? What if that commit adds bugs? Should the users blame/report about that version or that commit?
For those reasons, I'd much rather take no "version responsibility" as soon as a single commit has been added to an instance.
What about the version displayed on the client? Does this apply to the client as well?
You raise a good point and I believe versions should match in both the console and the UI.
If we want to push the 🚲 🏠 further, if not on a version, console could list the latest version + all commits added to that version, but we're going waaaaay too far! After all, we can't be doing the admin's job when it comes to using git 😅
So yeah, I definitely believe same on console + UI, and if admin wants more info, well, git log
is there for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove pkg
definition as it's no longer used in this file.
bdd3fec
to
ed6f062
Compare
@@ -1,14 +1,19 @@ | |||
'use strict'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(const
and let
)
ed6f062
to
5ab09db
Compare
So, the errors aside (because of the ES6 stuff, which we need to figure out), I like this. As far as I can see, it'll still show the npm version if it's installed from there? |
Yes. This PR is pretty redundant in terms of the amount of users who'll notice a change (I'd say we're probably down to a few ‰ 😄). |
if (_gitRef) { | ||
return _gitRef; | ||
} | ||
const commands = ["git describe HEAD", "git rev-parse --short HEAD"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just git describe HEAD
should be enough. It was introduced in [email protected].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, if the first command succeeds, the for
loop assigns stuff then continues to the next one, but if it fails, it... continues to the next one? 😅
Not sure that was the intent here.
Indeed, you can go with just one command, no need to over-complexify the code for this. Chances that the command is not available are way lower than not having git
at all, and it's fine to default to npm version otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll try all commands in the array, as soon as one succeeds it'll cache the result for subsequent invokations and then return the stdout. If the command fails it'll continue to the next one, and if all fails return null
.
44e190e
to
1871457
Compare
.execSync(`${command} 2> /dev/null`) | ||
.toString() | ||
.trim(); | ||
return _gitRef = gitRef; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please break this down into 2 statements:
_gitRef = gitRef;
return gitRef;
Hey @williamboman, thanks and good job consolidating versions! One more comment. Maybe a stretch, but we have to get started somewhere: how easy/hard would it be to write a couple tests for this? |
So firstly I think we need to agree on how to present the version number if the instance is running from source on a non-tagged commit. My preference is to use The other option would be to simply present the commit SHA (which is how it currently works). Then this PR would have to shift focus entirely to just consolidate version numbers throughout all interfaces.
Tests for printing version numbers in the CLI, or? |
1871457
to
4e09276
Compare
My preference goes the other way around for said confusion, and other reasons described in a comment above (mostly because of the info being displayed doesn't reflect reality too well: this is not the running version anymore!).
I actually think that such consolidation would be pretty great already! Actually, maybe worth doing a first pass with that consolidation, then in a further PR play with the other options? There are actually many ways git can report this and we might want to look into it, while the other goal (UI+console consistency) is a no-brainer.
I know, this is rather far-fetched and probably useless, but I'd love it if we could write tests for our app... So probably ignore my comment, but if you have a good and simple idea, I'll take it :-) |
I like the idea of showing a version and a git commit, as it actually gives you an idea where you are on git, rather than having to open the commit and guess from the previous commits. However, I don't use the version in the app enough to care. I understand the issues you have with it, @astorije and I won't argue it much. |
4e09276
to
f5e155e
Compare
I'm just annoyed by the version number being reported in the CLI when running from source, hence this PR. I don't feel very strongly about the semantics so I've shifted the focus of this PR to solely consolidate version numbers reported by the app. So effectively all this PR does now is change the output in the CLI when starting lounge; |
// Hack to defer execution of Helper.getGitCommit() | ||
toString() { | ||
return Helper.getGitCommit() ? Helper.getGitCommit() : pkg.version; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts? Feels a bit unnecessary to invoke Helper.getGitCommit()
for every command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you trying to check if Helper.getGitCommit
exists? Because you are invoking it either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I'm deferring execution of toString
until commander
wants to use it. Might break other stuff since we're not passing a string though.
The overhead of spawning a child process to get the git SHA is pretty much non-existent though, but it feels a bit unnecessary to do it for every single command (especially since it'll fail since most lounge installations are done through npm
, I assume).
62a9bc5
to
986ab64
Compare
} else { | ||
log.info(`The Lounge v${pkg.version} is now running`); | ||
} | ||
log.info(`Available on: ${protocol}://${host}:${config.port}/, in ${config.public ? "public" : "private"} mode`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the ,
after the url.
Could you also add the commit in https://github.com/thelounge/lounge/blob/2bb782fe81089bc634ae4946c68806a6d89b6e99/src/client.js#L255 |
e6c3c1a
to
d90465a
Compare
if (Helper.getGitCommit()) { | ||
log.info(`The Lounge is running from ${Helper.getVersion()}`); | ||
} else { | ||
log.info(`The Lounge ${Helper.getVersion()} is now running`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a particular fan of using the ES6 bollocks. The log
allows for concat by doing:
log.info("The Lounge", Helper.getVersion(), "is now running");
@@ -1,3 +1,6 @@ | |||
"use strict"; | |||
|
|||
const pkg = require('../package.json'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double quotes.
log.info("The Lounge v" + pkg.version + " is now running on", protocol + "://" + (config.host || "*") + ":" + config.port + "/", (config.public ? "in public mode" : "in private mode")); | ||
let protocol = config.https.enable ? "https" : "http"; | ||
let host = config.host || "*"; | ||
if (Helper.getGitCommit()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not even sure this different message is needed, could always just be printing the else
statement.
@@ -121,7 +116,7 @@ function index(req, res, next) { | |||
pkg, | |||
Helper.config | |||
); | |||
data.gitCommit = gitCommit; | |||
data.gitCommit = Helper.getGitCommit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the frontend just use Helper.getVersion()
too? Then the getGitCommit
function is only internal and getVersion
would be the single authority.
EDIT: It's required for a github link, nevermind.
2283cfe
to
442c284
Compare
@williamboman, could you rebase this PR with |
442c284
to
9921834
Compare
return null; | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not too happy this was merged with removed comments as mentioned below some time ago, but oh well.
consolidate version numbers throughout all interfaces
I find the current reporting of versions, both in the client and CLI, is a bit lackluster. For example, starting the server on latest
master
logs a message that (as of now) saysThe Lounge v2.0.0-pre.6 is now running ...
when in factmaster
is 30 commits ahead of pre-6.This PR consolidates version numbers reported in the client and the CLI. Effectively, this means there'll only be visible changes when starting lounge through the CLI;