kunwp1 commented on code in PR #4902:
URL: https://github.com/apache/texera/pull/4902#discussion_r3185211931
##########
frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts:
##########
@@ -936,13 +928,105 @@ export class ComputingUnitSelectionComponent implements
OnInit {
this.cdr.detectChanges();
});
};
+ }
+
+ private refreshUserPackages(index: number): void {
+ const env = this.pves[index];
- socket.onclose = event => {
- console.log("PVE WS closed", {
- code: event.code,
- reason: event.reason,
- wasClean: event.wasClean,
+ this.workflowPveService
+ .getUserPackages(this.selectedComputingUnit!.computingUnit.cuid,
env.name)
+ .pipe(untilDestroyed(this))
+ .subscribe({
+ next: pkgs => {
+ env.userPackages = env.userPackages = this.parsePackageRows(pkgs);
+ this.cdr.detectChanges();
+ },
+ error: (e: unknown) => console.error("Failed to refresh user
packages", e),
});
- };
+ }
+
+ createVirtualEnvironment(index: number): void {
+ const env = this.pves[index];
+ const trimmedName = env.name.trim();
+
+ if (!/^[a-zA-Z0-9]+$/.test(trimmedName)) {
+ this.notificationService.error("Environment name must contain only
letters and numbers.");
+ return;
+ }
+
+ if (env.isLocked) {
+ this.installUserPackages(index);
+ return;
+ }
+
+ const duplicateExists = this.pves.some((pve, i) => i !== index &&
(pve.name ?? "").trim() === trimmedName);
+
+ if (duplicateExists) {
+ this.notificationService.error("An environment with this name already
exists.");
+ return;
+ }
+
+ this.runPveWebSocket(index, "create", "Creating virtual environment...\n",
[], () => {
+ this.installUserPackages(index);
+ });
+ }
+
+ private installUserPackages(index: number): void {
+ const env = this.pves[index];
+
+ const systemPackageNames = new Set(this.systemPackages.map(pkg =>
pkg.name.trim().toLowerCase()));
+
+ const userPackageNames = new Set(env.userPackages.map(pkg =>
pkg.name.trim().toLowerCase()));
+
+ const skippedMessages: string[] = [];
+
+ const packageArray =
+ env.newPackages
+ ?.filter(pkg => pkg.name?.trim())
+ .filter(pkg => {
+ const packageName = pkg.name.trim().toLowerCase();
+
+ if (systemPackageNames.has(packageName)) {
+ skippedMessages.push(`[PVE] Skipped ${pkg.name}: already installed
as a system package.`);
Review Comment:
I remember you used a notification service to show an error. Can you do the
same thing here instead of showing the message on the output log. I feel like
showing the error messages in the output log is not very user-friendly.
##########
frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts:
##########
@@ -936,13 +928,105 @@ export class ComputingUnitSelectionComponent implements
OnInit {
this.cdr.detectChanges();
});
};
+ }
+
+ private refreshUserPackages(index: number): void {
+ const env = this.pves[index];
- socket.onclose = event => {
- console.log("PVE WS closed", {
- code: event.code,
- reason: event.reason,
- wasClean: event.wasClean,
+ this.workflowPveService
+ .getUserPackages(this.selectedComputingUnit!.computingUnit.cuid,
env.name)
+ .pipe(untilDestroyed(this))
+ .subscribe({
+ next: pkgs => {
+ env.userPackages = env.userPackages = this.parsePackageRows(pkgs);
+ this.cdr.detectChanges();
+ },
+ error: (e: unknown) => console.error("Failed to refresh user
packages", e),
});
- };
+ }
+
+ createVirtualEnvironment(index: number): void {
+ const env = this.pves[index];
+ const trimmedName = env.name.trim();
+
+ if (!/^[a-zA-Z0-9]+$/.test(trimmedName)) {
+ this.notificationService.error("Environment name must contain only
letters and numbers.");
+ return;
+ }
+
+ if (env.isLocked) {
+ this.installUserPackages(index);
+ return;
+ }
+
+ const duplicateExists = this.pves.some((pve, i) => i !== index &&
(pve.name ?? "").trim() === trimmedName);
+
+ if (duplicateExists) {
+ this.notificationService.error("An environment with this name already
exists.");
+ return;
+ }
+
+ this.runPveWebSocket(index, "create", "Creating virtual environment...\n",
[], () => {
+ this.installUserPackages(index);
+ });
+ }
+
+ private installUserPackages(index: number): void {
+ const env = this.pves[index];
+
+ const systemPackageNames = new Set(this.systemPackages.map(pkg =>
pkg.name.trim().toLowerCase()));
+
+ const userPackageNames = new Set(env.userPackages.map(pkg =>
pkg.name.trim().toLowerCase()));
+
+ const skippedMessages: string[] = [];
+
+ const packageArray =
+ env.newPackages
+ ?.filter(pkg => pkg.name?.trim())
+ .filter(pkg => {
+ const packageName = pkg.name.trim().toLowerCase();
+
+ if (systemPackageNames.has(packageName)) {
+ skippedMessages.push(`[PVE] Skipped ${pkg.name}: already installed
as a system package.`);
+ return false;
+ }
+
+ if (userPackageNames.has(packageName)) {
+ skippedMessages.push(`[PVE] Skipped ${pkg.name}: already installed
in this environment.`);
Review Comment:
Same here.
##########
frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts:
##########
@@ -936,13 +928,105 @@ export class ComputingUnitSelectionComponent implements
OnInit {
this.cdr.detectChanges();
});
};
+ }
+
+ private refreshUserPackages(index: number): void {
+ const env = this.pves[index];
- socket.onclose = event => {
- console.log("PVE WS closed", {
- code: event.code,
- reason: event.reason,
- wasClean: event.wasClean,
+ this.workflowPveService
+ .getUserPackages(this.selectedComputingUnit!.computingUnit.cuid,
env.name)
+ .pipe(untilDestroyed(this))
+ .subscribe({
+ next: pkgs => {
+ env.userPackages = env.userPackages = this.parsePackageRows(pkgs);
+ this.cdr.detectChanges();
+ },
+ error: (e: unknown) => console.error("Failed to refresh user
packages", e),
});
- };
+ }
+
+ createVirtualEnvironment(index: number): void {
+ const env = this.pves[index];
+ const trimmedName = env.name.trim();
+
+ if (!/^[a-zA-Z0-9]+$/.test(trimmedName)) {
+ this.notificationService.error("Environment name must contain only
letters and numbers.");
+ return;
+ }
+
+ if (env.isLocked) {
+ this.installUserPackages(index);
+ return;
+ }
+
+ const duplicateExists = this.pves.some((pve, i) => i !== index &&
(pve.name ?? "").trim() === trimmedName);
+
+ if (duplicateExists) {
+ this.notificationService.error("An environment with this name already
exists.");
+ return;
+ }
+
+ this.runPveWebSocket(index, "create", "Creating virtual environment...\n",
[], () => {
+ this.installUserPackages(index);
+ });
+ }
+
+ private installUserPackages(index: number): void {
+ const env = this.pves[index];
+
+ const systemPackageNames = new Set(this.systemPackages.map(pkg =>
pkg.name.trim().toLowerCase()));
+
+ const userPackageNames = new Set(env.userPackages.map(pkg =>
pkg.name.trim().toLowerCase()));
+
+ const skippedMessages: string[] = [];
+
+ const packageArray =
+ env.newPackages
+ ?.filter(pkg => pkg.name?.trim())
+ .filter(pkg => {
+ const packageName = pkg.name.trim().toLowerCase();
+
+ if (systemPackageNames.has(packageName)) {
+ skippedMessages.push(`[PVE] Skipped ${pkg.name}: already installed
as a system package.`);
+ return false;
+ }
+
+ if (userPackageNames.has(packageName)) {
+ skippedMessages.push(`[PVE] Skipped ${pkg.name}: already installed
in this environment.`);
+ return false;
+ }
+
+ return true;
+ })
+ .map(pkg => `${pkg.name.trim()}${pkg.version ?
`==${pkg.version.trim()}` : ""}`) ?? [];
+
+ if (skippedMessages.length > 0) {
+ this.pves[index].pipOutput = `${this.pves[index].pipOutput ?? ""}` +
skippedMessages.join("\n") + "\n";
+
+ this.updatePrettyPipOutput(index);
+ this.scrollToBottomOfPipModal(index);
+ }
+
+ if (packageArray.length === 0) {
+ this.pves[index].newPackages = [];
+ this.refreshUserPackages(index);
+ return;
+ }
+
+ this.runPveWebSocket(index, "install", "Installing user packages...\n",
packageArray, () => {
+ this.pves[index].newPackages = [];
+ this.refreshUserPackages(index);
+ });
+ }
+
+ private parsePackageRows(packages: string[]): PveUserPackageRow[] {
+ return packages.map(pkgStr => {
+ const [name, version] = pkgStr.split("==");
+ return {
+ name: name.trim(),
+ operator: "==" as const,
+ version: (version ?? "").trim(),
Review Comment:
Is it possible to show a version installed if the user doesn't specify a
version? I don't like keeping it empty.
##########
frontend/src/app/workspace/component/power-button/computing-unit-selection.component.html:
##########
Review Comment:
I feel like it's better to make the layout of showing user packages to be
consistent with showing system packages: Showing one header followed by
multiple packages
<img width="961" height="330" alt="Image"
src="https://github.com/user-attachments/assets/9fbdd5ea-e5fc-4a75-9400-cae9acc78744";
/>
##########
frontend/src/app/workspace/component/power-button/computing-unit-selection.component.ts:
##########
@@ -77,8 +77,16 @@ import { NzSliderComponent } from "ng-zorro-antd/slider";
import { NzAlertComponent } from "ng-zorro-antd/alert";
import { NzCollapseComponent, NzCollapsePanelComponent } from
"ng-zorro-antd/collapse";
+type PackageRow = {
+ name: string;
+ operator?: "==" | ">=" | "<=";
Review Comment:
Do you disagree on changing the variable name?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
