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

refactor: Add top-level version package #583

Merged
merged 8 commits into from
Jul 20, 2022

Conversation

orpheuslummis
Copy link
Contributor

@orpheuslummis orpheuslummis commented Jul 1, 2022

Relevant issue(s)

Resolves #349

Description

Add a central top-level version package where version information, of DefraDB itself, its protocols, and API versions, etc can be determined and used in other packages. Both the idea and discussion are, of course, up for discussion.

NOTES:

  • Cobra offers a --version flag which allows to provide version information on the root command, but a version command seems preferable.
  • This would be the place to later support version comparison, version-checking.

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Manual testing.

Specify the platform(s) on which this was tested:

  • MacOS
  • Debian Linux

@orpheuslummis orpheuslummis added refactor This issue specific to or requires *notable* refactoring of existing codebases and components action/no-benchmark Skips the action that runs the benchmark. labels Jul 1, 2022
@orpheuslummis orpheuslummis requested a review from a team July 1, 2022 14:22
@orpheuslummis orpheuslummis self-assigned this Jul 1, 2022
@orpheuslummis orpheuslummis added this to the DefraDB v0.3 milestone Jul 1, 2022
@codecov
Copy link

codecov bot commented Jul 1, 2022

Codecov Report

Merging #583 (cf689ca) into develop (c7257c4) will increase coverage by 0.01%.
The diff coverage is 88.88%.

❗ Current head cf689ca differs from pull request most recent head f0cd8d4. Consider uploading reports for the commit f0cd8d4 to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #583      +/-   ##
===========================================
+ Coverage    57.09%   57.11%   +0.01%     
===========================================
  Files          122      122              
  Lines        14646    14652       +6     
===========================================
+ Hits          8362     8368       +6     
  Misses        5567     5567              
  Partials       717      717              
Impacted Files Coverage Δ
api/http/router.go 100.00% <ø> (ø)
client/dockey.go 51.16% <66.66%> (ø)
core/net/protocol.go 77.77% <100.00%> (+44.44%) ⬆️

@orpheuslummis
Copy link
Contributor Author

Closed probably because of Cmd-Enter closing it? Re-opened now.

@orpheuslummis
Copy link
Contributor Author

Didn't use the new-in-1.18 capability https://pkg.go.dev/runtime/debug#BuildInfo because we currently on 1.17.
This means instead of relying on BuildInfo + build system, here we just rely on build system.

To have it work properly via Docker we copy the .git directory in the build process - to avoid this, a pure BuildInfo approach could be used in future at the probable cost of less information included.

@orpheuslummis orpheuslummis marked this pull request as ready for review July 5, 2022 01:58
@orpheuslummis orpheuslummis added the area/cli Related to the CLI binary label Jul 5, 2022
Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

Looks pretty good. I look forward to discussing my suggestions bellow 😁.

cli/version.go Outdated Show resolved Hide resolved
cli/version.go Outdated Show resolved Hide resolved
version/version.go Outdated Show resolved Hide resolved
version/version.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

LGTM 🌞

@jsimnz
Copy link
Member

jsimnz commented Jul 13, 2022

I have been reviewing the PR, and I had several pending comments, and I realized they are all along the same theme, so I figured I would just make a single comment now about the nature of this package, we can discuss/resolve the comment, and depending on that descision, I can submit my actual review, or you make the necessary changes.

I'm trying to determine the scope of this top-level version package. The original intent was just as a place to hook in the compiler LDFLAGS at build time to hook into the build version info (date, time, git commit, tag, go version, etc). But this PR has moved almost anything related to versionining here.

I personally like the simplicity of the build-only approach, additionally, any version information that is currently package-specific (ie: ProtocolID, HTTPAPI, DocKey, DocKeyNamespace) has been migrated into this single package, but I believe it makes sense semantically to keep the component-specific version information along side where those components exist.

TLDR; I don't think version information beyond the build info should be in this package.

This is entirely open for debate.

@fredcarle
Copy link
Collaborator

I personally like the simplicity of the build-only approach, additionally, any version information that is currently package-specific (ie: ProtocolID, HTTPAPI, DocKey, DocKeyNamespace) has been migrated into this single package, but I believe it makes sense semantically to keep the component-specific version information along side where those components exist.

I thought this had been discussed prior to the PR so I didn't want to argue the migration. But I do prefer having the package specific versions residing inside those packages.

@orpheuslummis
Copy link
Contributor Author

I agree with the guideline of colocating information.

In this case, it seems to me that some in-package version information is to be used externally. Specifically, I can think of at least 3 consumers of the information.

  • HTTP API
  • RPC
  • version and version-check commands of CLI

Because there is additional logic relevant to versioning, such as serialization, comparison, and potentially assisting with a deprecation policy, I saw version package as a natural place for it.

It is also possible for each package-specific version information to remain in its package, but to be gathered by a version.

@orpheuslummis
Copy link
Contributor Author

I'll update in light of today's discussion

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

The change looks good. Only a few things I think should be improved and then it will be good to go :)

api/http/router.go Outdated Show resolved Hide resolved
client/dockey.go Outdated Show resolved Hide resolved
client/dockey.go Outdated Show resolved Hide resolved
core/net/protocol.go Show resolved Hide resolved
core/net/protocol.go Show resolved Hide resolved
@orpheuslummis
Copy link
Contributor Author

Thanks @fredcarle

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes Orphée!

@orpheuslummis orpheuslummis force-pushed the orpheus/feat/top-level-version branch from ff8cd26 to aadc87c Compare July 19, 2022 21:30
Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

Thanks for the excellent work on this Orhpheee!

In addition to the comments / suggestion I left I was wondering specifically for the non-json command (i.e. defradb version) that we are going from this:

image

To:

image

I know we planned to add global color support in future so that is fine, but maybe we can make the text output slightly more pretty =D?

Here is a suggestion (I don't think we need branch):

DefraDB's Version Information:
  *  branch       : master
  *  build commit : e4328e0
  *  release date : 2022-03-07T00:12:07Z
  *  version tag  : 0.2.1

Other Version Information:
  *  dockey versions : 1
  *  golang          : 1.17.8 linux/amd64
  *  http api        : v0
  *  net protocol    : /defra/0.0.1

version/version.go Outdated Show resolved Hide resolved
@orpheuslummis
Copy link
Contributor Author

orpheuslummis commented Jul 20, 2022

@shahzadlone not sure how to reply directly to your latest suggestion about making the output prettier.

What do you think of this suggestion? We can imagine later on adding on color when shell environment supports it.

$ defradb version
defradb v0.3.0 (3ff5db5a 2022-07-23)

Further version information:
- HTTP API: v0
- P2P multicodec: /defra/0.0.1
- DocKey: 1
- Go: 1.18.4 darwin/arm64

@fredcarle
Copy link
Collaborator

What do you think of this suggestion? We can imagine later on adding on color when shell environment supports it.

Adding colour is pretty easy if you want to add it.

@shahzadlone
Copy link
Member

shahzadlone commented Jul 20, 2022

@shahzadlone not sure how to reply directly to your latest suggestion about making the output prettier.

What do you think of this suggestion? We can imagine later on adding on color when shell environment supports it.

$ defradb version
defradb v0.3.0 (3ff5db5a 2022-07-23)

Further version information:
- HTTP API: v0
- P2P multicodec: /defra/0.0.1
- DocKey: 1
- Go: 1.18.4 darwin/arm64

I like this better than original, but why not indentation for the lines after the title and * instead of - :P?
I do find the line by line more readable even for defradb version information, but I leave the final decision up to you.
Here is another suggestion:

DefraDB Version:
  *  build commit : e4328e0
  *  git release  : 0.2.1
  *  release date : 2022-03-07T00:12:07Z

Other Version Information:
  *  dockey       : 1
  *  go           : 1.17.8 linux/amd64
  *  http api     : v0
  *  net protocol : /defra/0.0.1

@orpheuslummis
Copy link
Contributor Author

What do you think of this suggestion? We can imagine later on adding on color when shell environment supports it.

Adding colour is pretty easy if you want to add it.

Yes but it should be a different PR to enable a global color flag and color in multiple places.

@orpheuslummis
Copy link
Contributor Author

orpheuslummis commented Jul 20, 2022

My preference and suggestion now is to use a one-liner for the human version:

defradb v0.3.0 (1ec604b7 2022-07-25) built with Go 1.18.4 darwin/arm64

Thoughts?

@shahzadlone
Copy link
Member

My preference and suggestion now is to use a one-liner for the human version:

defradb v0.3.0 (1ec604b7 2022-07-25) built with Go 1.18.4 darwin/arm64

Thoughts?

Too long of a line I feel like. Probably nitpicking at this point but If someone's running defradb on a remote server and have a tiny terminal screen, version info is going to be on one long horizontal line that will be wrapped into multiple lines? Previous suggestions keep them readable even for a small terminal window (I don't see the appeal of it all being squished to 1 line).

@orpheuslummis
Copy link
Contributor Author

My preference and suggestion now is to use a one-liner for the human version:

defradb v0.3.0 (1ec604b7 2022-07-25) built with Go 1.18.4 darwin/arm64

Thoughts?

Too long of a line I feel like. Probably nitpicking at this point but If someone's running defradb on a remote server and have a tiny terminal screen, version info is going to be on one long horizontal line that will be wrapped into multiple lines? Previous suggestions keep them readable even for a small terminal window (I don't see the appeal of it all being squished to 1 line).

I expect most contemporary terminal displays to show be able to show ~70 characters without issue, therefore I don't see it as a problem.

@fredcarle
Copy link
Collaborator

Too long of a line I feel like. Probably nitpicking at this point but If someone's running defradb on a remote server and have a tiny terminal screen, version info is going to be on one long horizontal line that will be wrapped into multiple lines? Previous suggestions keep them readable even for a small terminal window (I don't see the appeal of it all being squished to 1 line).

That would be an awfully small terminal lol. The one liner is fine to me. I've seen many other projects use it. But I don't mind multiline either. At this point it's really a matter of personal preferences.

@orpheuslummis
Copy link
Contributor Author

I suggest that an advanced user that cares about which DocKey versions and netprotocol are supported can obtain it with via the JSON output.

@shahzadlone
Copy link
Member

shahzadlone commented Jul 20, 2022

I expect most contemporary terminal displays to show be able to show ~70 characters without issue, therefore I don't see it as a problem.

characters alone isn't an accurate measurement, size and type of font can change that somewhat.

That would be an awfully small terminal lol. The one liner is fine to me. I've seen many other projects use it. But I don't mind multiline either. At this point it's really a matter of personal preferences.

I like one liner as long as it's not too long (for example the way golang does it).

Here is the output on my 17inch screen with one tmux split:

image

I suggest that an advanced user that cares about which DocKey versions and netprotocol are supported can obtain it with via the JSON output.

I would strongly suggest that the only difference between json and text format to be the format, not the level of information we share. They should both show the same amount of information.

@orpheuslummis
Copy link
Contributor Author

The output you show, found below, is displaying version information on a development branch. It has 99 characters in this case, but depends on the branch name.

defradb dev-orpheus/feat/top-level-version (df06f3cf 2022-07-20) built with Go 1.18.4 darwin/arm64

The vast majority of users will use a stable release and will see something like the following which in this case has 71-2 characters, as it uses tag from the master branch.

defradb v0.3.0 (df06f3cf 2022-07-20) built with Go 1.18.4 darwin/arm64

@orpheuslummis
Copy link
Contributor Author

orpheuslummis commented Jul 20, 2022

The status quo is to provide the useful information for the vast majority of use cases as a one-liner.

An option is to expose the parity of information between human & JSON, for the advanced users, under a --full flag.

@shahzadlone
Copy link
Member

The status quo is to provide the useful information for the vast majority of use cases as a one-liner.

An option is to expose the parity of information between human & JSON, for the advanced users, under a --full flag.

By default maybe we print the longer version (for both json and text) and have a flag -short for the shorter one line format?

As mentioned these are nitpicks (non-blocking). I leave the final style choice up to you =)

@orpheuslummis
Copy link
Contributor Author

The status quo is to provide the useful information for the vast majority of use cases as a one-liner.
An option is to expose the parity of information between human & JSON, for the advanced users, under a --full flag.

By default maybe we print the longer version (for both json and text) and have a flag -short for the shorter one line format?

As mentioned these are nitpicks (non-blocking). I leave the final style choice up to you =)

The version information for human is used when debugging collaboratively or deployed nodes, or when creating issues. I suggest the short version should be the default, as the extra information is most likely derivable via the short version information.

I'll add a --full or --long flag then I suggest it can be merged.

@orpheuslummis orpheuslummis force-pushed the orpheus/feat/top-level-version branch from cf689ca to f0cd8d4 Compare July 20, 2022 17:11
@orpheuslummis orpheuslummis merged commit dd28534 into develop Jul 20, 2022
@orpheuslummis orpheuslummis deleted the orpheus/feat/top-level-version branch July 20, 2022 17:15
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/cli Related to the CLI binary refactor This issue specific to or requires *notable* refactoring of existing codebases and components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI: version cmd in light of CLI overhaul
4 participants