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

Remove baseapp dependency on the version package #4250

Merged

Conversation

alessio
Copy link
Contributor

@alessio alessio commented May 2, 2019

The version package is meant to be a convenience utility
that provides SDK consumers with a ready-to-use version
command that produces app's versioning information from
flags passed at compile time.
It will not make sense anymore for the baseapp package
to depend on the version package once gaia will have been
migrated away from the SDK main repository as we neither
want to make assumptions nor set expectations on downstream
apps buildsystems. Thus BaseApp now provides SetAppVersion()
and AppVersion() to to allow SDK consumers to set BaseApp's
version information string once the struct is initialised.

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added a relevant changelog entry: clog add [section] [stanza] [message]
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

The version package is meant to be a convenience utility
that provides SDK consumers with a ready-to-use version
command that produces app's versioning information from
flags passed at compile time.
It will not make sense anymore for the baseapp package
to depend on the version package once gaia will have been
migrated away from the SDK main repository as we neither
want to make assumptions nor set expectations on downstream
apps buildsystems. Thus BaseApp now provides SetAppVersion()
and AppVersion() to to allow SDK consumers to set BaseApp's
version information string once the struct is initialised.
@codecov
Copy link

codecov bot commented May 2, 2019

Codecov Report

Merging #4250 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4250      +/-   ##
==========================================
+ Coverage   58.91%   58.95%   +0.04%     
==========================================
  Files         215      215              
  Lines       14482    14489       +7     
==========================================
+ Hits         8532     8542      +10     
+ Misses       5310     5307       -3     
  Partials      640      640

@alessio alessio marked this pull request as ready for review May 2, 2019 02:16
@alessio alessio requested review from fedekunze and sabau May 2, 2019 02:16
Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

Looks good pending Name being set for gaia

Commit = ""
Version = ""
// Application's name
Name = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR should probably show an example for how the gaia Name is set here (within the cmd/gaia) - I'm assuming you just forgot this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming you just forgot this

I did not: https://github.com/cosmos/cosmos-sdk/pull/4250/files#diff-b67911656ef5d18c4ae36cb6741b7965R47

All these variables are set via build flags. I didn't provide examples as there were any before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've improved the version package docs

func (app *BaseApp) AppVersion() string { return app.appVersion }

// SetAppVersion sets the application's version string.
func (app *BaseApp) SetAppVersion(v string) { app.appVersion = v }
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably live in options.go and check for sealed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

++

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

TestedACK.

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Agree with @alexanderbez comment that it should check if the version is sealed before setting it

func (app *BaseApp) AppVersion() string { return app.appVersion }

// SetAppVersion sets the application's version string.
func (app *BaseApp) SetAppVersion(v string) { app.appVersion = v }
Copy link
Collaborator

Choose a reason for hiding this comment

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

++

@alessio alessio requested a review from fedekunze May 2, 2019 18:20
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM

@alessio alessio merged commit 38f9312 into master May 2, 2019
@alessio alessio deleted the alessio/gaia-split-prep-remove-baseapp-dep-on-version-pkg branch May 2, 2019 19:37
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.

4 participants