Skip to content

Conversation

@anmolsachan
Copy link
Member

@anmolsachan anmolsachan commented Sep 20, 2018

tendrl-bug-id: #575
bugzilla: 1631260
Signed-off-by: Anmol Sachan anmol13694@gmail.com

@anmolsachan anmolsachan requested a review from a team as a code owner September 20, 2018 13:53
@anmolsachan
Copy link
Member Author

@nthomas-redhat @shtripat @GowthamShanmugam Please review.


username = raw_input("Please enter the grafana admin_username "
"default-[%s]: " % default_user) or default_user
password = raw_input("Please enter the grafana admin_password default"
Copy link
Member

Choose a reason for hiding this comment

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

Would you be able to hide this while user enters the password? That would be better.

Copy link
Member Author

@anmolsachan anmolsachan Sep 20, 2018

Choose a reason for hiding this comment

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

@shtripat yes, it can be hidden, I thought of showing it so that user can cross-verify the default vs what he knows. Should I hide it ?

Copy link
Member

Choose a reason for hiding this comment

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

I am fine with any approach as this is just a tool, but personally I feel it should be hidden. Anyway if wrong password passed, later stage it would fail with error.

Copy link
Member Author

@anmolsachan anmolsachan Sep 20, 2018

Choose a reason for hiding this comment

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

@shtripat Please review, I added command line args
It will work like: python delete_dashboards.py --username=admin --password=pass

If the args are not supplied, it will take from grafana.ini

print "\n Clearing grafana dashboards \n"
delete_dashboards(server_ip=default_ip, user=username,
password=password)
print "\n Complete -- Please start tendrl-monitoring-integration service"
Copy link
Member

Choose a reason for hiding this comment

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

And carbon-cache service?

Copy link
Member Author

Choose a reason for hiding this comment

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

@shtripat Rest of the steps will be supplied with the documentation.

Copy link
Member

Choose a reason for hiding this comment

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

But in command line as well we should to restart tendrl-monitoring-integration and carbon-cache and order should be start carbon-cache and then tendrl-monitoring-integration I feel.

@nthomas-redhat what you say?

Otherwise code look good to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

@shtripat We will not stop carbon cache as there is no interference with it. Should we stop carbon-cache ?

Copy link
Member

@shtripat shtripat Sep 21, 2018

Choose a reason for hiding this comment

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

If no interference then its fine

@codecov
Copy link

codecov bot commented Sep 20, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@817d977). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #574   +/-   ##
========================================
  Coverage          ?   44.1%           
========================================
  Files             ?      41           
  Lines             ?    2351           
  Branches          ?     352           
========================================
  Hits              ?    1037           
  Misses            ?    1261           
  Partials          ?      53
Impacted Files Coverage Δ
...nitoring_integration/upgrades/delete_dashboards.py 0% <0%> (ø)

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 817d977...50427c4. Read the comment docs.

Copy link
Member

@shtripat shtripat left a comment

Choose a reason for hiding this comment

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

LGTM

@cybernth
Copy link
Contributor

cybernth commented Sep 21, 2018

@anmolsachan , i am not seeing the spec file changes here?
This file to be placed in use/local/bin/tendrl
Also is it a good idea to integrate the execution of script integrated with rpm spec itself? Or tendrl-ansible?
@shtripat , @TimothyAsirJeyasing , @mbukatov ?

@cybernth
Copy link
Contributor

Also is it a good idea to integrate the execution of script integrated with rpm spec itself?
@shtripat , @TimothyAsirJeyasing ?

For now let's go with the manual route; later we can think of get that integrated with ansible or rpm scripts itself. @anmolsachan , please raise an issue to track

@anmolsachan
Copy link
Member Author

@nthomas-redhat I have made changes according to your suggestion. The script is located at /usr/bin/tendrl-monitoring-integration-upgrade and can be run as " /usr/bin/tendrl-monitoring-integration-upgrade --username=username --password= password". It gets installed with the rpm. Verified.

Copy link
Member

@shtripat shtripat left a comment

Choose a reason for hiding this comment

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

LGTM
@nthomas-redhat please check once and do the needful.

@cybernth
Copy link
Contributor

@nthomas-redhat I have made changes according to your suggestion. The script is located at /usr/bin/tendrl-monitoring-integration-upgrade and can be run as " /usr/bin/tendrl-monitoring-integration-upgrade --username=username --password= password". It gets installed with the rpm. Verified.

Couple of questions:

  1. who places the file into usr/local/bin? I don't see any code doing that part.
  2. What happens if its a fresh install and not an upgrade?
  3. What happens if the grafana server is not running?

@shtripat
Copy link
Member

who places the file into usr/local/bin? I don't see any code doing that part.

Its setup.py job

What happens if its a fresh install and not an upgrade?

This tool gets deployed but no need to use it

What happens if the grafana server is not running?

If we try to execute the tool it will fail

@anmolbabu correct me if wrong here.

tendrl-bug-id: Tendrl#575
bugzilla: 1631260
Signed-off-by: Anmol Sachan <anmol13694@gmail.com>

Making delete_dashboards.py as an  executable
@cybernth cybernth merged commit ecf7299 into Tendrl:master Sep 21, 2018
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.

3 participants