Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

*: add version check before start #311

Merged
merged 8 commits into from
May 29, 2020
Merged

*: add version check before start #311

merged 8 commits into from
May 29, 2020

Conversation

3pointer
Copy link
Collaborator

What problem does this PR solve?

Resolve #301

What is changed and how it works?

Add cluster version check when br start

Check List

Tests

  • Unit test

Related changes

  • Need to cherry-pick to the release branch

Release Note

  • Feature: Add version check at beginning in order to avoid version mismatch.

@kennytm
Copy link
Collaborator

kennytm commented May 26, 2020

Please include a flag to disable version check (e.g. --check-requirements=false).

kennytm
kennytm reviewed May 26, 2020
View reviewed changes
pkg/utils/version.go Outdated Show resolved Hide resolved
pkg/utils/version.go Outdated Show resolved Hide resolved
YuJuncen
YuJuncen reviewed May 26, 2020
View reviewed changes
Copy link
Collaborator

@YuJuncen YuJuncen left a comment

Choose a reason for hiding this comment

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

Rest LGTM

pkg/utils/version.go Show resolved Hide resolved
@3pointer
Copy link
Collaborator Author

/run-integration-test

@3pointer @kennytm
Update pkg/utils/version.go
42863df 
Co-authored-by: kennytm <kennytm@gmail.com>
@3pointer
Copy link
Collaborator Author

/run-integration-test

5 similar comments
@3pointer
Copy link
Collaborator Author

/run-integration-test

@3pointer
Copy link
Collaborator Author

/run-integration-test

@3pointer
Copy link
Collaborator Author

/run-integration-test

@3pointer
Copy link
Collaborator Author

/run-integration-test

@3pointer
Copy link
Collaborator Author

/run-integration-test

@YuJuncen
Copy link
Collaborator 

[2020-05-26T12:08:45.673Z] Error: None is not in dotted-tri format

Maybe PD returns "None" at Version Field?

@codecov
Copy link

codecov bot commented May 26, 2020 •
edited
Loading

Codecov Report

Merging #311 into master will decrease coverage by 0.87%.
The diff coverage is 76.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #311      +/-   ##
==========================================
- Coverage   72.44%   71.56%   -0.88%     
==========================================
  Files          48       48              
  Lines        5277     5135     -142     
==========================================
- Hits         3823     3675     -148     
- Misses        976      989      +13     
+ Partials      478      471       -7
Impacted Files Coverage Δ
pkg/conn/conn.go 77.82% <50.00%> (-4.87%) ⬇️
pkg/task/restore.go 54.31% <50.00%> (ø)
pkg/task/common.go 65.54% <71.42%> (-7.50%) ⬇️
pkg/utils/version.go 87.50% <80.64%> (-1.79%) ⬇️
pkg/task/backup.go 61.74% <100.00%> (ø)
pkg/task/backup_raw.go 56.52% <100.00%> (ø)
pkg/task/restore_raw.go 59.37% <100.00%> (ø)
pkg/restore/import.go 67.95% <0.00%> (-3.87%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1edbe26...6e053e3. Read the comment docs.

@3pointer
Copy link
Collaborator Author 

[2020-05-26T12:08:45.673Z] Error: None is not in dotted-tri format

I found it's BRReleaseVersion is None, but CI has build BR with ldflags, after a few attempt to adjust CI, It didn't work well, So I decide to add skip to work around.

YuJuncen
YuJuncen reviewed May 27, 2020
View reviewed changes
Copy link
Collaborator

@YuJuncen YuJuncen left a comment

Choose a reason for hiding this comment

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

Rest LGTM

pkg/task/common.go
@@ -116,6 +118,9 @@ func DefineCommonFlags(flags *pflag.FlagSet) {
flags.Uint64(flagRateLimitUnit, utils.MB, "The unit of rate limit")
_ = flags.MarkHidden(flagRateLimitUnit)

flags.Bool(flagCheckRequirement, true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Boolean flags seems must use the form --some=false to disable(see here), otherwise when it appears, it would be set to true.
Maybe --skip-check-reqirements here would be better? Since we must use --check-requirements=false to skip check requirements if we let the default value be true on this flag.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO this is fine since we've got --checksum=false already.

--skip-check-requirements would be a "negative flag".

@overvenus
Copy link
Member 

[2020-05-26T12:08:45.673Z] Error: None is not in dotted-tri format

I found it's BRReleaseVersion is None, but CI has build BR with ldflags, after a few attempt to adjust CI, It didn't work well, So I decide to add skip to work around.

You can add -ldflags '$(LDFLAGS)' to the build_for_integration_test target in Makefile.

kennytm
kennytm reviewed May 28, 2020
View reviewed changes
Copy link
Collaborator

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

lGTM

@kennytm kennytm added the status/LGT1 LGTM1 label May 28, 2020
@overvenus
Copy link
Member

Please remove .DS_Store files https://github.com/pingcap/br/blob/1a825f01f7e61fc0ac6b3d2408de7b75116d89f3/.DS_Store

Rest LGTM

@3pointer
Copy link
Collaborator Author

/run-integration-test

@kennytm kennytm merged commit 3e05ea6 into pingcap:master May 29, 2020
@kennytm
Copy link
Collaborator

kennytm commented May 29, 2020

/run-cherry-picker

@sre-bot
Copy link
Contributor

sre-bot commented May 29, 2020

cherry pick to release-3.1 failed

1 similar comment
@sre-bot
Copy link
Contributor

sre-bot commented May 29, 2020

cherry pick to release-3.1 failed

@sre-bot sre-bot mentioned this pull request May 29, 2020
Merged
@sre-bot
Copy link
Contributor

sre-bot commented May 29, 2020

cherry pick to release-4.0 in PR #326

kennytm added a commit that referenced this pull request Jun 2, 2020
@sre-bot @3pointer @kennytm
*: add version check before start (#311) (#326)
a1c1d5d 
* add version check

* Update pkg/utils/version.go

Co-authored-by: kennytm <kennytm@gmail.com>

* fix test

* add flag to control check

* address comment

* fix ci

* remove DS_Store

Co-authored-by: luancheng <luancheng@pingcap.com>
Co-authored-by: kennytm <kennytm@gmail.com>
3pointer added a commit to 3pointer/br that referenced this pull request Jun 5, 2020
@3pointer @kennytm @overvenus
*: add version check before start (pingcap#311)
7c5c187 
* add version check

* Update pkg/utils/version.go

Co-authored-by: kennytm <kennytm@gmail.com>

* fix test

* add flag to control check

* address comment

* fix ci

* remove DS_Store

Co-authored-by: kennytm <kennytm@gmail.com>
Co-authored-by: Neil Shen <overvenus@gmail.com>
3pointer added a commit to 3pointer/br that referenced this pull request Jun 5, 2020
@3pointer @kennytm @overvenus
*: add version check before start (pingcap#311)
44aa39a 
* add version check

* Update pkg/utils/version.go

Co-authored-by: kennytm <kennytm@gmail.com>

* fix test

* add flag to control check

* address comment

* fix ci

* remove DS_Store

Co-authored-by: kennytm <kennytm@gmail.com>
Co-authored-by: Neil Shen <overvenus@gmail.com>
@3pointer 3pointer mentioned this pull request Jun 5, 2020
Merged
overvenus pushed a commit that referenced this pull request Jun 5, 2020
@3pointer
*: add version check before start (#311) (#335)
1639c24
@kennytm kennytm mentioned this pull request Sep 10, 2020
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@YuJuncen YuJuncen YuJuncen left review comments

@kennytm kennytm kennytm approved these changes

@overvenus overvenus Awaiting requested review from overvenus

Assignees
No one assigned
Labels
needs-cherry-pick-release-3.1 needs-cherry-pick-release-4.0 status/LGT1 LGTM1 type/feature-request New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BR add version check at beginning.
5 participants
@3pointer @kennytm @YuJuncen @overvenus @sre-bot