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

feature/cli-commands-touchup #62

Merged
merged 22 commits into from
Jun 18, 2021
Merged

feature/cli-commands-touchup #62

merged 22 commits into from
Jun 18, 2021

Conversation

zivkovicmilos
Copy link
Contributor

Description

This PR fixes some cli command outputs, as well as standardizes the way some commands are being called.

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have tested this code
  • I have updated the README and other relevant documents (guides...)
  • I have added sufficient documentation both in code, as well as in the READMEs

@zivkovicmilos zivkovicmilos self-assigned this Jun 9, 2021
@zivkovicmilos zivkovicmilos marked this pull request as ready for review June 14, 2021 15:40
lazartravica
lazartravica suggested changes Jun 15, 2021
View reviewed changes
Copy link
Contributor

@lazartravica lazartravica left a comment

Choose a reason for hiding this comment

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

Excellent stuff!

Just one question, I would appreciate to hear your thoughts on this: Why did we move away from the subcommands, e.g. ibft init towards ibft-init?

Unless we have a very good argument for that I would advise against doing this as we would need to update the docs and communicate with the users that they need to change the way they do everything with polygon-sdk.

@zivkovicmilos zivkovicmilos mentioned this pull request Jun 15, 2021
Merged
@zivkovicmilos
Copy link
Contributor Author

@lazartravica
We moved away from the subcommands model because it's easier to keep everything uniform, same as with geth.
The commands also now follow the UCB utility guidelines.

There is already a PR waiting on the documentation repo, with the changes from this PR.

Vuksan
Vuksan suggested changes Jun 15, 2021
View reviewed changes
Copy link
Contributor

@Vuksan Vuksan left a comment

Choose a reason for hiding this comment

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

Nice job!

Take a look at the suggestions I left - feel free to discuss anything you don't agree with.

command/commands.go Outdated Show resolved Hide resolved
command/ibft_propose.go Outdated Show resolved Hide resolved
command/ibft_propose.go Outdated Show resolved Hide resolved
command/ibft_snapshot.go Outdated Show resolved Hide resolved
command/ibft_snapshot.go Outdated Show resolved Hide resolved
command/peers_add.go Outdated Show resolved Hide resolved
command/peers_add.go Outdated Show resolved Hide resolved
command/status.go Outdated Show resolved Hide resolved
command/server/command.go Outdated Show resolved Hide resolved
command/server/command.go Outdated Show resolved Hide resolved
Vuksan
Vuksan approved these changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@Vuksan Vuksan left a comment

Choose a reason for hiding this comment

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

Kudos for refactoring testserver as well 👏

I left a comment to rename a parameter, which isn't a showstopper. If you agree, you can do that and merge this one.

e2e/framework/testserver.go Outdated Show resolved Hide resolved
Kourin1996
Kourin1996 approved these changes Jun 17, 2021
View reviewed changes
Copy link
Contributor

@Kourin1996 Kourin1996 left a comment

Choose a reason for hiding this comment

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

Thank you for your improvements and fixing e2e codes accordingly.
I noticed a small thing, it'd be great if you could fix if necessary.
Thanks.

command/helper/helper.go Outdated Show resolved Hide resolved
lazartravica
lazartravica approved these changes Jun 18, 2021
View reviewed changes
Copy link
Contributor

@lazartravica lazartravica left a comment

Choose a reason for hiding this comment

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

you did an awesome job!

everything works and it's much clearer now.

@zivkovicmilos zivkovicmilos merged commit c4e5c79 into develop Jun 18, 2021
@zivkovicmilos zivkovicmilos deleted the feature/cli-commands-touchup branch June 30, 2021 10:05
igorcrevar pushed a commit that referenced this pull request Jun 12, 2023
@ferranbt
Merge pull request #62 from umbracle/feature-fixe
e0d0d37 
Fix topics without non-indexed fields
goran-ethernal pushed a commit that referenced this pull request Jan 24, 2024
@dependabot
Bump github.com/aws/aws-sdk-go from 1.46.1 to 1.49.7 (#62)
2d83ec9 
Bumps [github.com/aws/aws-sdk-go](https://github.com/aws/aws-sdk-go) from 1.46.1 to 1.49.7.
- [Release notes](https://github.com/aws/aws-sdk-go/releases)
- [Commits](aws/aws-sdk-go@v1.46.1...v1.49.7)

---
updated-dependencies:
- dependency-name: github.com/aws/aws-sdk-go
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
goran-ethernal pushed a commit that referenced this pull request Apr 11, 2024
@bane
Fix Load Tests Notifications (#62)
9e828c4 
* Deploy Network

* Add Permissions

* Minor changes

* Concurrency change

* Destroy Network

* Minor changes

* Minor changes

* Fix commit sha

* Minor changes

* Add Tests

* Disable Tests temporarily

* Nightly Build

* Minor fixes

* Big changes

* Minor changes

* Minor changes

* Minor changes

* Notification fixes

* Notification changes

* Minor changes

* Permissions fixes

* Minor changes

* Notification changes

* Minor changes

* Display commit on notifications

* Minor changes

* Minor fixes

* Logs notification fixes

* Minor changes

* Minor changes

* Minor changes

* Notifications fixes

* Minor changes

* Update actions version

* Deployment fixes

* Minor changes

* Minor changes

* Build repository fixes

* Load Test notification text changes

* EIP-1559; Check if the network is already deployed; Restore Blade Data

* Notifications fixes; Pass blade_repository when deploying the network

* Terraform fixes

* Minor changes

* Minor changes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Notification fixes

* Restore data fixes

* Fix condition

* Fix the RPC URL when the network is already deployed

* Fix the RPC URL when the network is already deployed

* Fix condition

* Fix condition

* Fix condition

* Fix condition

* Fix condition

* Fix condition

* Fix condition

* Fix condition

* Fix condition

* Fix condition

* Fix condition

* Fix condition

* Fix condition

* Fix condition

* Fix condition

* Fix condition

* Fix condition

* Fix condition

* Fix condition

* Fixed when the network is first time deploying

* Fixed upload logs

* Fix: null URL if the network is destroyed but the state file exists

* Main notification changes

* Fix RPC URL; Fix Logs and Data upload

* Minor fix

* Minor fix

* Notification fixes

* Deploy fixes

* Big changes

* Condition fixes

* Version changes

* Test fixes

* Change golangci-lint-action source

* Change ec2-github-runner version; Always execute Build Blade

* Minor changes

* Lint fixes

* Nightly test

* Minor fixes

* Fix Linter bug

* Release changes

* goreleaser fixes

* goreleaser fixes

* goreleaser fixes

* goreleaser fixes

* goreleaser fixes

* docker repository name fixes

* Minor fixes

* Warrning fixes

* docker repository name fixes

* Add organization variable

* Rename Tests to CI; Minor changes

* Fix Load Test notifications

* Fix Load Test notifications

* Minor changes

* Minor fixes

* Fix Load Tests notifications
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kourin1996 Kourin1996 Kourin1996 approved these changes

@Vuksan Vuksan Vuksan approved these changes

@lazartravica lazartravica lazartravica approved these changes

Assignees

@zivkovicmilos zivkovicmilos

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zivkovicmilos @lazartravica @Vuksan @Kourin1996