Skip to content
This repository has been archived by the owner on Jan 24, 2022. It is now read-only.

Push only required contract during create command #1426

Merged
merged 4 commits into from
Feb 7, 2020

Conversation

ylv-io
Copy link
Contributor

@ylv-io ylv-io commented Feb 6, 2020

Fixes #1250. Will push only request contract by create command. The same functionality theoretically can be implemented for update command as well but it will require quite a bit of rearchitecting due to update command accepting options like all/alias/address as well as these options being resolved down the call stack of the update command.

Copy link
Contributor

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

The change looks good, Igor! Could I ask you to add a few tests on test/scripts/push.test.js? Thanks!

@@ -89,9 +94,9 @@ async function runActionIfRequested(externalOptions: any): Promise<void> {
return action(options);
}

async function runActionIfNeeded(contractName: string, network: string, options: any): Promise<void> {
async function runActionIfNeeded(contractAlias: string, network: string, options: any): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This contractName was not even used, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct.

if (contractAlias) pushArguments.contractAliases = [contractAlias];

if (!options.skipTelemetry)
await Telemetry.report('push', (pushArguments as unknown) as Record<string, unknown>, interactive);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious: why was this cast needed when changing from { [key: string]: unknown } to Record<string, unknown> in the report method definition? I thought that both were semantically the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cast is needed not because of the change of report method but because of change pushParams to type PushParams which doesn't have required index signature.

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -122,13 +122,13 @@ export default class NetworkController {
}

// DeployerController
public async push(reupload = false, force = false): Promise<void | never> {
public async push(aliases: string[], { reupload = false, force = false } = {}): Promise<void | never> {
Copy link
Contributor

Choose a reason for hiding this comment

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

aliases should be string[] | undefined, and the same in _contractsListForPush. It's not necessary because we're not using strict mode yet, but I'd like us to be explicit about it so it will be easier to migrate to strict mode in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I was thinking there are all the null checks. I completely forgot about that our tsconfig is not in a strict mode. We should bump it to it as fast as possible.

Copy link
Contributor

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

These are proper tests. Great work Igor!!

@ylv-io ylv-io merged commit 7235c9a into master Feb 7, 2020
@mergify mergify bot deleted the feature/selective-push branch February 7, 2020 16:15
@darshan0071990
Copy link

I have updated cli sdk & upgrades to version 2.7.1. I have 2 different contracts let say Contract A {} and Contract B {}. Both the contracts are added and part of the contracts object of the project.json file. When I run oz create in interactive mode or oz create ContractA, it always deploys ContractA and ContractB together even after specifying only 1 contract which I need to deploy.

Looks like it still not fixed with the new release or am I doing something wrong with create.

@frangio
Copy link
Contributor

frangio commented Feb 13, 2020

Hi @darshan0071990. This was not included in the 2.7 release. It will be part of the 2.8 release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unrelated contracts get deployed when using create
4 participants