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

support rendering template when set "--local" option #1596

Merged
merged 3 commits into from
Nov 1, 2021

Conversation

makocchi-git
Copy link
Contributor

@makocchi-git makocchi-git commented Oct 25, 2021

What problem does this PR solve?

tiup cluster template prints example yaml file.
I think that it is more useful if users can specify global values or servers in topology.yaml.
This PR enables generating sample topology.yaml with specify variables.

What is changed and how it works?

When set --local, it renders a template with some overridden variables such as global.user, global.group, etc by setting --user, --group and so on together.

This is sample output:

$ tiup cluster template --local --user ubuntu
...
global:
  # # The OS user who runs the tidb cluster.
  user: "ubuntu"
...

=======

$ tiup cluster template --local --user ubuntu --pd-servers 127.0.0.1,127.0.0.2
...
global:
  # # The OS user who runs the tidb cluster.
  user: "ubuntu"
...
pd_servers:
  - host: 127.0.0.1
  - host: 127.0.0.2
...

And also support tiup dm template --local.

$ tiup dm template --local --user ubuntu
...
global:
  # # The OS user who runs the tidb cluster.
  user: "ubuntu"
...

=======

$ tiup dm template --local --user ubuntu --master-servers m1,m2,m3
...
global:
  # # The OS user who runs the tidb cluster.
  user: "ubuntu"
...
master_servers:
  - host: m1
  - host: m2
  - host: m3
...

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:

Support rendering example topology.yaml with specifying variables.

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Oct 25, 2021

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • AstroProfundis

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot requested review from birdstorm and lonng October 25, 2021 12:24
@ti-chi-bot ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 25, 2021
@AstroProfundis
Copy link
Contributor

Thanks for the PR! This is indeed a very useful feature. However, I feel the arguments are getting a little too complex. How about:

  • Set default values for the skeleton struct
  • Change our builtin examples to templates
  • If no other arguments are set, use the default values to render the template
  • If any custom argument is set, use the user input values to render the template (along with default ones)

So that the --skeleton argument is no longer needed, and all other argument are shortened as the --skeleton-xxx prefixes are also not needed.

@AstroProfundis AstroProfundis added category/usability Categorizes issue or PR as a usability enhancement. component/cluster Issues about the tiup-cluster component good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. type/new-feature Categorizes pr as related to a new feature. labels Oct 26, 2021
@AstroProfundis
Copy link
Contributor

And... Would you like to also add the same feature to tiup-dm? It would be better to adjust tiup-dm and tiup-cluster together, but it's also totally OK if you don't want to.

@AstroProfundis AstroProfundis added this to the v1.7.0 milestone Oct 26, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 26, 2021

Codecov Report

Merging #1596 (b25d926) into master (956504c) will increase coverage by 35.01%.
The diff coverage is 41.79%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1596       +/-   ##
===========================================
+ Coverage   15.71%   50.72%   +35.01%     
===========================================
  Files         149      290      +141     
  Lines       15098    26486    +11388     
===========================================
+ Hits         2372    13434    +11062     
+ Misses      12208    11032     -1176     
- Partials      518     2020     +1502     
Flag Coverage Δ
cluster 43.87% <53.57%> (?)
dm 26.06% <33.33%> (?)
integrate 50.72% <41.79%> (+35.01%) ⬆️
playground 13.47% <ø> (?)
tiup 15.71% <ø> (ø)
unittest ∅ <ø> (∅)

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

Impacted Files Coverage Δ
components/dm/command/template.go 37.03% <33.33%> (ø)
components/cluster/command/template.go 40.35% <53.57%> (ø)
components/cluster/command/reload.go 80.64% <0.00%> (ø)
pkg/cluster/manager/transfer.go 51.13% <0.00%> (ø)
components/dm/command/destroy.go 63.15% <0.00%> (ø)
components/cluster/command/telemetry.go 24.00% <0.00%> (ø)
components/playground/instance/tikv.go 67.85% <0.00%> (ø)
components/dm/command/exec.go 87.50% <0.00%> (ø)
components/cluster/command/check.go 86.66% <0.00%> (ø)
components/cluster/command/prune.go 84.61% <0.00%> (ø)
... and 235 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 956504c...b25d926. Read the comment docs.

@makocchi-git
Copy link
Contributor Author

makocchi-git commented Oct 27, 2021

@AstroProfundis Thank you for your comments.

Set default values for the skeleton struct

Skelton default values are set by cobra functions. (same as --full or --local, etc)

Change our builtin examples to templates

I think that it is a bit hard to render a template completely. (especially full template)
Because there are too many structured options in pd_servers or tidb_servers, etc.
Of course I know skelton template can't render a complete one, so I think it is useful for customizing some parameters in demonstration environment, for example.
This is the reason why I created a skelton way as a new rendering option.

So I suggest and agree replacing --local to skelton rendering.
(I agree too that --skelton-xxx is a lttile bit annoying....😅)
I'd like to hear your opinions.

tiup-dm

OK, I'll try.

@AstroProfundis
Copy link
Contributor

So I suggest and agree replacing --local to skelton rendering.
(I agree too that --skelton-xxx is a lttile bit annoying....sweat_smile)
I'd like to hear your opinions.

Sounds good. I'm just feeling that --skeleton and --skeleton-xxx are redundant and may be confusing the user, I think it's ok to let those --user xxx arguments only work for --local template as of now, we can add more rendering ability to other templates in the future and that don't seems to me that would broke anything.

@ti-chi-bot ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 27, 2021
@makocchi-git
Copy link
Contributor Author

Now supports tiup-dm template --local and add some unit tests.

@makocchi-git makocchi-git changed the title support generating template from skelton support rendering template when set "--local" option Oct 27, 2021
@makocchi-git
Copy link
Contributor Author

rebased

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 29, 2021
@AstroProfundis
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 0cbbf61

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 29, 2021
@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Oct 30, 2021
@makocchi-git
Copy link
Contributor Author

Sorry. I fixed ioutil package.

@AstroProfundis
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: b25d926

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 1, 2021
@ti-chi-bot ti-chi-bot merged commit 9200644 into pingcap:master Nov 1, 2021
@makocchi-git makocchi-git deleted the feature/gen-by-tmpl branch November 1, 2021 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category/usability Categorizes issue or PR as a usability enhancement. component/cluster Issues about the tiup-cluster component good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. type/new-feature Categorizes pr as related to a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants