-
Notifications
You must be signed in to change notification settings - Fork 116
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
simplify naming #925
simplify naming #925
Conversation
This just changes the binary names? I like the idea |
yep. I just need to clean it up and such, should be able to get to it tomorrow. It is inspired by typing: interchain-security-pd too many times :) |
I'm getting test failures on this even after re-running. Can you check that the e2e tests all have the updated binary names? |
ah that's why it is still on draft. |
interchain-security-pd => provider
hmmmm I thought i had this working.... |
@jtremback please note that we could debug this directly on github, if we changed the automated tests to use the --verbose flag as seen in #867 that would:
... idk exactly how things work at Informal but at Notional we've noticed very consistently that machines are dramatically cheaper than the people who program them. |
For reference, 5/5 vietnamese undersea cables have some kind of fault right now. Thus, i am still downloading docker containers. |
LGTM but looks like some merge conflicts snuck in |
Set this to draft, but once the merge conflicts are fixed I'm happy to review |
Co-authored-by: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com>
Co-authored-by: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com>
Co-authored-by: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com>
yeah the idea was def to make binary names shorter, any other name changes, were done for the sake of consistency is there a specific instance where this is an issue? Happy to address |
restore d's
@@ -14,7 +14,7 @@ import ( | |||
"github.com/cometbft/cometbft/libs/log" | |||
|
|||
appConsumer "github.com/cosmos/interchain-security/v3/app/consumer" | |||
appConsumerDemocracy "github.com/cosmos/interchain-security/v3/app/consumer-democracy" | |||
appConsumerDemocracy "github.com/cosmos/interchain-security/v3/app/democracy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this what you were unsure of @smarshall-spitzbart ?
Merge conflicts snuck in again, can merge once it's fixed though |
@jtremback hey sorry about the delay here. Baby gap sir. |
I've merged main, hopefully this fixes the cometmocktest. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just one nit below.
@@ -63,7 +63,7 @@ OR | |||
gaiad query provider validator-provider-key <consumer-chain-id> consumervalcons1e....123asdnoaisdao | |||
``` | |||
|
|||
You must use a `valcons` address. You can obtain it by querying your node on the consumer `consumerd tendermint show-address` | |||
You must use a `valcons` address. You can obtain it by querying your node on the consumer `consumer tendermint show-address` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be consumerd
.
COPY --from=is-builder /go/bin/providerd /usr/local/bin/providerd | ||
COPY --from=is-builder /go/bin/consumerd /usr/local/bin/consumerd | ||
COPY --from=is-builder /go/bin/democracyd /usr/local/bin/democracyd | ||
COPY --from=is-builder /go/bin/sovereignd /usr/local/bin/sovereignd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also change sovereign to standalone? Calling a non-ICS chain sovereign, entails that consumer chains don't have sovereignty, which is not true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I'm good with this change sir
Closing this as it is out of sync with main. It's easier to redo the PR than to rebase. |
Description
Binary names and paths were too long.
This PR:
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change