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

Add 'vscode-kafka.api.saveclusters' and 'vscode-kafka.api.deleteclusters' commands #189

Merged
merged 1 commit into from
Jul 26, 2021

Conversation

fbricon
Copy link
Collaborator

@fbricon fbricon commented May 12, 2021

Signed-off-by: Fred Bricon [email protected]

@@ -231,4 +205,35 @@ export async function configureDefaultClusters(clusterSettings: ClusterSettings)
];
}

export async function addClusters(clusters: Cluster[], clusterSettings: ClusterSettings, explorer: KafkaExplorer) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we should prolly move that out from the wizard namespace

Copy link
Collaborator

Choose a reason for hiding this comment

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

For me addCluster should be renamed to saveCluster and it's a command handler. I suggest that you create in commands/clusters.ts a new class

export class SaveClusterCommandHandler {

    constructor(private clusterSettings: ClusterSettings, private explorer: KafkaExplorer) {
    }

    async execute(clusters: Cluster[]): Promise<void> {
        try {
            // Save collected clusters in settings.
            let createdClusterNames = '';
            for (const cluster of clusters) {
                this.clusterSettings.upsert(cluster);
                if (createdClusterNames !== '') {
                    createdClusterNames += '\', \'';
                }
                createdClusterNames += cluster.name;
            }
            vscode.window.showInformationMessage(`${clusters.length > 1 ? `${clusters.length} clusters` : 'Cluster'} '${createdClusterNames}' created successfully`);
    
            // Refresh the explorer
            this.explorer.refresh();
    
            // Selecting the created cluster is done with TreeView#reveal
            // 1. Show the treeview of the explorer (otherwise reveal will not work)
            this.explorer.show();
            // 2. the reveal() call must occur within a timeout(),
            // while waiting for a fix in https://github.com/microsoft/vscode/issues/114149
            setTimeout(() => {
                if (clusters) {
                    this.explorer.selectClusterByName(clusters[0].name);
                }
            }, 1000);
        }
        catch (error) {
            showErrorMessage(`Error while creating cluster`, error);
        }
    }
}

and you consume it in extension.ts and wizard both.

@angelozerr
Copy link
Collaborator

We should really add a Developer section to explain how to extend vscode-kafka with #129 and you new command.

@fbricon fbricon force-pushed the create-clusters-command branch from a3f77ff to 171b6c3 Compare July 5, 2021 14:38
@fbricon fbricon changed the title Add 'vscode-kafka.api.addclusters' command Add 'vscode-kafka.api.saveclusters' and 'vscode-kafka.api.deleteclusters' commands Jul 5, 2021
@fbricon fbricon force-pushed the create-clusters-command branch 2 times, most recently from 7289a8f to b280e14 Compare July 6, 2021 08:31
@fbricon fbricon marked this pull request as ready for review July 6, 2021 08:35
@fbricon fbricon force-pushed the create-clusters-command branch from b280e14 to cd5b51e Compare July 15, 2021 15:02
}
createdClusterNames += cluster.name;
}
let createdClusterNames = getNames(clusters);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think wizard création should wait for SaveClusterCommandHandler command and after we could consume it to savez clusters

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah I had something like that before the new wizard, but after rebasing, I just wanted to push something that compiles

@fbricon fbricon force-pushed the create-clusters-command branch 5 times, most recently from 905eaba to 3ccc52a Compare July 23, 2021 16:24
@@ -196,7 +197,7 @@ function createEditClusterForm(cluster: Cluster | undefined, clusterSettings: Cl
cluster = createCluster(data);
}
// Save cluster
saveCluster(true, data, cluster, clusterSettings, clientAccessor, explorer, selectCluster);
await saveCluster(data, cluster);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have managed select cluster in SaveCommandHandler, it's nice, but you should remove all method which use selectCluster parameter;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

You missed some code.

@fbricon fbricon force-pushed the create-clusters-command branch from 3ccc52a to 9eb0298 Compare July 26, 2021 10:00
@@ -19,34 +65,51 @@ export class AddClusterCommandHandler {
}

async execute(selectCluster = false): Promise<void> {
openClusterWizard(this.clusterSettings, this.clientAccessor, this.explorer, this.context, selectCluster);
openClusterWizard(this.clusterSettings, this.clientAccessor, this.explorer, this.context);
Copy link
Collaborator

Choose a reason for hiding this comment

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

async execute(): Promise<void> {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@fbricon fbricon force-pushed the create-clusters-command branch from 9eb0298 to d5e60b6 Compare July 26, 2021 10:13
@fbricon fbricon force-pushed the create-clusters-command branch from d5e60b6 to 4f75b10 Compare July 26, 2021 10:14
@angelozerr angelozerr merged commit bf3f9f0 into jlandersen:master Jul 26, 2021
@angelozerr
Copy link
Collaborator

It works great, thanks @fbricon !

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.

2 participants