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

Fix the dependencies field to match the specification #953

Merged
merged 4 commits into from
Dec 2, 2020

Conversation

lucklove
Copy link
Member

@lucklove lucklove commented Dec 1, 2020

What problem does this PR solve?

As https://github.com/pingcap/tiup/blame/master/doc/design/manifest.md#L256 specified, the dependencies filed should be map[string]string. But in the current implementation, it's []string, fortunately, this field is not used up until now and it's aways a null in the manifest json file, which adopted both []string and map[string]string. But in the future, we may use this field, so it's better to correct it as early as possible.

What is changed and how it works?

  • change []string to map[string]string
  • upgrade the spec version from v0.1.0 to v0.1.1

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change
  • Has persistent data change

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Release notes:

NONE

@ti-chi-bot ti-chi-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 1, 2020
@lucklove lucklove added type/bug-fix Categorizes PR as a bug-fix and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 1, 2020
@lucklove lucklove added this to the v1.3.0 milestone Dec 1, 2020
@ti-chi-bot ti-chi-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 1, 2020
@codecov-io
Copy link

codecov-io commented Dec 1, 2020

Codecov Report

Merging #953 (fc9a59a) into master (601a3e0) will decrease coverage by 3.80%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #953      +/-   ##
==========================================
- Coverage   55.81%   52.00%   -3.81%     
==========================================
  Files         263      263              
  Lines       19513    19513              
==========================================
- Hits        10891    10148     -743     
- Misses       6889     7724     +835     
+ Partials     1733     1641      -92     
Flag Coverage Δ
cluster 38.08% <0.00%> (-5.25%) ⬇️
dm 24.34% <0.00%> (+0.04%) ⬆️
integrate 46.20% <0.00%> (-3.82%) ⬇️
playground 20.28% <0.00%> (ø)
tiup 16.83% <0.00%> (+0.02%) ⬆️
unittest 23.06% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/repository/v1manifest/manifest.go 52.98% <0.00%> (ø)
pkg/repository/v1manifest/types.go 76.92% <ø> (ø)
components/cluster/command/check.go 6.37% <0.00%> (-72.82%) ⬇️
pkg/cluster/task/limits.go 0.00% <0.00%> (-68.75%) ⬇️
pkg/cluster/task/sysctl.go 0.00% <0.00%> (-66.67%) ⬇️
components/cluster/command/audit.go 27.27% <0.00%> (-54.55%) ⬇️
pkg/cluster/operation/check.go 0.00% <0.00%> (-51.95%) ⬇️
pkg/cluster/task/rmdir.go 0.00% <0.00%> (-50.00%) ⬇️
pkg/cluster/operation/operation.go 34.78% <0.00%> (-43.48%) ⬇️
components/cluster/command/rename.go 25.00% <0.00%> (-41.67%) ⬇️
... and 30 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 601a3e0...fc9a59a. Read the comment docs.

Copy link
Contributor

@9547 9547 left a comment

Choose a reason for hiding this comment

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

/lgtm

@lucklove
Copy link
Member Author

lucklove commented Dec 2, 2020

/merge

@ti-chi-bot
Copy link
Member

@lucklove: you cannot merge your own PR.

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the tidb-community-bots/ti-community-prow repository.

@9547
Copy link
Contributor

9547 commented Dec 2, 2020

/merge

@ti-chi-bot
Copy link
Member

@9547: adding 'status/cam-merge' is restricted to committers in list.

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the tidb-community-bots/ti-community-prow repository.

@lucklove
Copy link
Member Author

lucklove commented Dec 2, 2020

/hold

@ti-chi-bot ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 2, 2020
@lucklove
Copy link
Member Author

lucklove commented Dec 2, 2020

@9547 I found a bug ):

@lucklove
Copy link
Member Author

lucklove commented Dec 2, 2020

We can't upgrade spec_version at this time, until #960 resolved.

@lucklove
Copy link
Member Author

lucklove commented Dec 2, 2020

/hold cancel

@ti-chi-bot ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 2, 2020
@lucklove lucklove merged commit 4e336b2 into pingcap:master Dec 2, 2020
@lucklove lucklove deleted the fix-deps branch December 2, 2020 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/bug-fix Categorizes PR as a bug-fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants