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

Added seed node update scripts #1821

Merged
merged 9 commits into from
Aug 1, 2018
Merged

Added seed node update scripts #1821

merged 9 commits into from
Aug 1, 2018

Conversation

greg-szabo
Copy link
Member

  • Linked to github-issue with discussion and accepted design
  • Updated all relevant documentation (docs/)
  • Updated all relevant code comments
  • Ran tests
  • Added entries in PENDING.md
  • Updated cmd/gaia and examples/

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Jul 25, 2018

Codecov Report

Merging #1821 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           develop   #1821   +/-   ##
=======================================
  Coverage     63.5%   63.5%           
=======================================
  Files          118     118           
  Lines         7006    7006           
=======================================
  Hits          4449    4449           
  Misses        2301    2301           
  Partials       256     256

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Thanks @greg-szabo! Just left a few remarks :-)

@@ -0,0 +1,17 @@
[Unit]
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worthwhile renaming this? This seems to start the LCD, should we rename this to lcd.service and related nomenclature?

Copy link
Member Author

Choose a reason for hiding this comment

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

The naming was deliberate because this is supposed to be temporary. A CLI should not act as a server, the LCD functionality needs to be broken out to somewhere else.

I keep naming the services based on the binary name (I used to do it differently and ran into issues).

The more immediate reason why this should not be renamed right now is that the scripts were tested and there's no easy "refactor" button for ansible scripts. If I rename it, I'll have to go through all scripts and check if I ever used it. Everything has to be re-tested again to make sure 1. I didn't break anything and 2. it was updated everywhere. I'd rather leave it like this and move away from it completely as soon as the LCD functionality was separated.
(A more obscure case of the naming is in a template for logz.io so it's not just the tasks you have to update but everything that could depend on the name.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough.

@@ -0,0 +1,15 @@
---

- name: Copy binary
Copy link
Contributor

Choose a reason for hiding this comment

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

See below 👍

@if ! [ -f $(HOME)/.ssh/id_rsa.pub ]; then ssh-keygen ; fi
@if [ -z "`file $(BINARY) | grep 'ELF 64-bit'`" ]; then echo "Please build a linux binary using 'make build-linux'." ; false ; fi
@if [ -z "`file $(GAIACLI_BINARY) | grep 'ELF 64-bit'`" ]; then echo "Please build a linux binary using 'make build-linux'." ; false ; fi
cd remote/ansible && ANSIBLE_HOST_KEY_CHECKING=False ansible-playbook -i inventory/ec2.py -l "tag_Environment_$(CLUSTER_NAME)" -u centos -b -e BINARY=$(BINARY) -e GAIACLI_BINARY=$(GAIACLI_BINARY) -e UNSAFE_RESET_ALL=$(UNSAFE_RESET_ALL) upgrade-gaia.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

Hate to be nitpicky here, but can we break up the long command onto new lines via \?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I've updated it in the code. I'll have to rewrite all long commands like that - which I didn't do yet.
Also, I can't push the change to the branch, it seems my access was revoked.

@@ -31,6 +31,7 @@ FEATURES
* [cosmos-sdk-cli] Added support for cosmos-sdk-cli tool under cosmos-sdk/cmd
* This allows SDK users to initialize a new project repository.
* [tests] Remotenet commands for AWS (awsnet)
* [networks] Added ansible scripts to upgrade seed nodes on a network
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this created from an existing issue? I believe the consensus is to add the issue # here, but if there is none, that's okay :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

No, there was no issue associated with this - at least not in the cosmos-sdk repo. There might be one in the devops repo but that's a private repo. Adding the issue number here from there wouldn't help the community at all.
I think the correct solution is to open issues on the cosmos-sdk repo regarding infrastructure scripts - which is additional administrative overhead, but doable.

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Changes LGTM once merge conflicts are resolved 👍

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

utACK

@cwgoes cwgoes merged commit 179b8f9 into develop Aug 1, 2018
@cwgoes cwgoes deleted the greg/seeds-infra branch August 1, 2018 01:13
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