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

Monitoring install scripts #3985

Merged
merged 5 commits into from
Feb 20, 2020
Merged

Monitoring install scripts #3985

merged 5 commits into from
Feb 20, 2020

Conversation

freimair
Copy link
Contributor

Completes first part of #3916.

  • cleans up the seednode install script
  • install script for monitoring servers with collectd
  • install script for the networksize metric

I tested the script for general monitoring using a fresh ubuntu 18.04 LTS installation. I could not test the script for the networksize metric as thoroughly, as there is no install script for a price node available yet.

@freimair freimair requested a review from cbeams as a code owner February 18, 2020 11:18
@freimair freimair requested a review from wiz February 18, 2020 11:19
@ripcurlx
Copy link
Contributor

@freimair Could you have a look at https://app.codacy.com/gh/bisq-network/bisq/pullRequest?prid=5023490. If there are false positives please let me know and I'll adapt the checks. Thanks!

@ripcurlx ripcurlx added the is:priority PR or issue marked with this label is up for compensation label Feb 18, 2020
@freimair freimair force-pushed the monitoring_install_script branch 2 times, most recently from 79eb307 to 3d4ba9a Compare February 18, 2020 11:53
@freimair
Copy link
Contributor Author

@freimair Could you have a look at https://app.codacy.com/gh/bisq-network/bisq/pullRequest?prid=5023490. If there are false positives please let me know and I'll adapt the checks. Thanks!

you beat me to that. I already fixed the issues. And nope, the checks seem fine. You get these issues if you copy and paste your way through life.

Is there a way to use codacy before creating the PR? (because no force-pushes, no fix commits, yadda yadda...)

@ripcurlx
Copy link
Contributor

Is there a way to use codacy before creating the PR? (because no force-pushes, no fix commits, yadda yadda...)

The only way would be if you enable it on your fork yourself to run on every branch you push.

@freimair freimair force-pushed the monitoring_install_script branch 2 times, most recently from 69df120 to c090416 Compare February 18, 2020 16:40
@wiz
Copy link
Contributor

wiz commented Feb 19, 2020

@freimair see this line for a workaround for the codacy issue https://github.com/bisq-network/bisq-explorer/blob/master/install_bsq_explorer_debian.sh#L56

@ripcurlx
Copy link
Contributor

@freimair see this line for a workaround for the codacy issue https://github.com/bisq-network/bisq-explorer/blob/master/install_bsq_explorer_debian.sh#L56

If you guys think this Codacy remark is not correct, then I can just deactivate this single check.

@freimair freimair force-pushed the monitoring_install_script branch from c090416 to 0d17bb6 Compare February 19, 2020 11:12
@freimair
Copy link
Contributor Author

should all be good now. My bad.

cbeams
cbeams reviewed Feb 19, 2020
View reviewed changes
Copy link
Contributor

@cbeams cbeams left a comment

Choose a reason for hiding this comment

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

I was pinged here for a code review because I'm the pricenode code owner, but the changes there are minimal and do not affect production code, so I'm giving a my OK from the pricenode side, but am abstaining from a larger review.

wiz
wiz suggested changes Feb 20, 2020
View reviewed changes
wiz
wiz suggested changes Feb 20, 2020
View reviewed changes
wiz
wiz approved these changes Feb 20, 2020
View reviewed changes
Copy link
Contributor

@wiz wiz left a comment

Choose a reason for hiding this comment

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

Tested 👍 on fresh Ubuntu 18 install using seednode installer + collectd installer

ripcurlx
ripcurlx approved these changes Feb 20, 2020
View reviewed changes
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

utACK

Based on #3985 (review)

@ripcurlx ripcurlx merged commit 7943026 into bisq-network:master Feb 20, 2020
@ripcurlx ripcurlx added this to the v1.2.8 milestone Feb 26, 2020
@freimair freimair mentioned this pull request Mar 11, 2020
Closed
@ripcurlx ripcurlx mentioned this pull request Mar 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbeams cbeams cbeams left review comments

@ripcurlx ripcurlx ripcurlx approved these changes

@wiz wiz wiz approved these changes

Assignees
No one assigned
Labels
is:priority PR or issue marked with this label is up for compensation
Projects
None yet
Milestone
v1.2.8
Development

Successfully merging this pull request may close these issues.
4 participants
@freimair @ripcurlx @wiz @cbeams