-
Notifications
You must be signed in to change notification settings - Fork 138
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
Chore: port ConsumerRemovalProposal
handler to main
#429
Conversation
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
Nice catch, thanks! |
dc973ca
to
1cf7297
Compare
Added integration tests. |
|
||
//#nosec G204 -- Bypass linter warning for spawning subprocess with cmd arguments. | ||
bz, err = exec.Command("docker", "exec", tr.containerConfig.instanceName, | ||
"/bin/bash", "-c", fmt.Sprintf(`echo '%s' > %s`, jsonStr, "/temp-proposal.json")).CombinedOutput() |
Check failure
Code scanning / CodeQL
Potentially unsafe quoting
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.
closed this alert, seen it before. Shouldn't matter for tests
// queryConsumerChainRemoved checks if provided consumerChain | ||
// was removed from the list of consumer chains on the provider. | ||
// If a chain was not removed a panic will be raised. | ||
// getConsumerChains returns a list of consumer chains that're being secured by the provider chain, |
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.
Thanks!
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.
couple small nits, added them myself, LGTM 🚀
@@ -82,7 +82,7 @@ func stepsStartChains(consumerName string, setupTransferChan bool) []Step { | |||
state: State{ | |||
chainID("provi"): ChainState{ | |||
Proposals: &map[uint]Proposal{ | |||
1: ConsumerProposal{ | |||
1: ConsumerAdditionProposal{ |
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.
@MSalopek great opportunity to improve these consumer chain addition tests by adding in your new ConsumerChains: &map[chainID]bool{"consu": true}
state accessor!
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
Port
ConsumerRemovalProposal
proposal handler and cli command from v0.1.4 to main