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.

Sign up for GitHub

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

Jump to bottom

feat(cluster/spec): check alertmanager,prometheus's config #1074

Merged
merged 11 commits into from
Jan 29, 2021
Merged

feat(cluster/spec): check alertmanager,prometheus's config #1074

merged 11 commits into from
Jan 29, 2021

Conversation

9547
Copy link
Contributor

@9547 9547 commented Jan 16, 2021

What problem does this PR solve?

Check Alertmanager,Prometheus' config after InitConfig, should be merged after #1073

What is changed and how it works?

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/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 16, 2021
@9547 9547 force-pushed the feature/check-prom-alertmanager-config branch from 3a27058 to 20434e4 Compare January 16, 2021 05:31
@codecov-io
Copy link

codecov-io commented Jan 16, 2021 •
edited
Loading

Codecov Report

Merging #1074 (c30aefb) into master (74dfe6f) will decrease coverage by 25.45%.
The diff coverage is 5.55%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1074       +/-   ##
===========================================
- Coverage   55.73%   30.28%   -25.46%     
===========================================
  Files         283      269       -14     
  Lines       20106    18680     -1426     
===========================================
- Hits        11207     5657     -5550     
- Misses       7167    12167     +5000     
+ Partials     1732      856      -876
Flag Coverage Δ
cluster ?
dm ?
integrate 21.34% <2.77%> (-28.79%) ⬇️
playground 20.89% <ø> (ø)
tiup 16.31% <2.77%> (-0.03%) ⬇️
unittest 22.92% <5.55%> (+0.11%) ⬆️

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

Impacted Files Coverage Δ
pkg/cluster/spec/alertmanager.go 8.47% <0.00%> (-58.20%) ⬇️
pkg/cluster/spec/grafana.go 18.75% <0.00%> (-50.00%) ⬇️
pkg/cluster/spec/server_config.go 54.36% <0.00%> (-13.02%) ⬇️
pkg/cluster/spec/prometheus.go 17.28% <20.00%> (-62.10%) ⬇️
pkg/cluster/embed/autogen_pkger.go 100.00% <100.00%> (ø)
pkg/meta/paths.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/utils/utils.go 0.00% <0.00%> (-100.00%) ⬇️
components/dm/main.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/logger/log/log.go 0.00% <0.00%> (-100.00%) ⬇️
components/cluster/main.go 0.00% <0.00%> (-100.00%) ⬇️
... and 183 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 74dfe6f...c30aefb. Read the comment docs.

@9547 9547 changed the title feat(cluster/spec): check alert,anager,prometheus's config feat(cluster/spec): check alertmanager,prometheus's config Jan 16, 2021
@lucklove
Copy link
Member

It seems the config check failed

@9547 9547 mentioned this pull request Jan 18, 2021
Closed
@9547
Copy link
Contributor Author

9547 commented Jan 18, 2021

Hi @lucklove Is there any documents about the usage of the Ti Robot, such as I want to merge PR #A after #B...

@lucklove
Copy link
Member

Hi @lucklove Is there any documents about the usage of the Ti Robot, such as I want to merge PR #A after #B...

https://book.prow.tidb.io/#/

Here is all the commands, I think.

@9547
Copy link
Contributor Author

9547 commented Jan 22, 2021

/hold

@ti-chi-bot ti-chi-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 22, 2021
@9547 9547 force-pushed the feature/check-prom-alertmanager-config branch from 20434e4 to 04a17fc Compare January 26, 2021 14:57
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 26, 2021
@9547 9547 force-pushed the feature/check-prom-alertmanager-config branch from 04a17fc to c30aefb Compare January 28, 2021 15:54
@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 28, 2021
@9547 9547 force-pushed the feature/check-prom-alertmanager-config branch from c30aefb to 5190918 Compare January 28, 2021 16:14
@9547
Copy link
Contributor Author

9547 commented Jan 28, 2021

/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 Jan 28, 2021
@9547
Copy link
Contributor Author

9547 commented Jan 28, 2021

/cc @lucklove PTAL

@ti-chi-bot ti-chi-bot requested a review from lucklove January 28, 2021 23:26
@ti-chi-bot
Copy link
Member

@9547: GitHub didn't allow me to request PR reviews from the following users: PTAL.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @lucklove PTAL

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 kubernetes/test-infra repository.

@lucklove
Copy link
Member

/lgtm

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • lucklove

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 writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

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

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 5190918

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 29, 2021
@ti-chi-bot ti-chi-bot merged commit 859e5fc into pingcap:master Jan 29, 2021
mianhk pushed a commit to mianhk/tiup that referenced this pull request Jan 29, 2021
@jsvisa @mianhk
feat(cluster/spec): check alertmanager,prometheus's config (pingcap#1074
7546cfd 
)

* feat(cluster/spec): check alertmanager,prometheus's config

* fix(cluster/spec): convert to ctxt.Executor

* fix(cluster/spec): use find cp instead of cp *.json(zsh compatible)

* enhance(cluster/spec): use find cp instead of cp for dm's grafana,prometheus

* fix(cluster/spec): index error

* feat(cluster/spec): add prometheus testcase

* feat(*): rm not used dm/prometheus.yml.tpl

* feat(cluster/spec): test Prometheus localDir

* fix(template/prometheus): if local rules were given, system rules won't auto copyed

* feat(clutster): make pkger

* feat(tests/cluster): ensure tidb exists in prometheus.yml
@9547 9547 deleted the feature/check-prom-alertmanager-config branch April 6, 2021 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@breezewish breezewish Awaiting requested review from breezewish

@july2993 july2993 Awaiting requested review from july2993

@lucklove lucklove Awaiting requested review from lucklove

Assignees
No one assigned
Labels
size/L Denotes a PR that changes 100-499 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@9547 @codecov-io @lucklove @ti-chi-bot @jsvisa