Skip to content

Conversation

Xemka
Copy link
Contributor

@Xemka Xemka commented Jan 29, 2021

  1. Добавлена функциональность для считывания всех сервисов всех подключенных пользователей, проверка их на доступность и отправка пользователям статусов этих сервисов

Copy link
Member

@ilyamore88 ilyamore88 left a comment

Choose a reason for hiding this comment

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

Ещё пофикси ts. Тогда потестирую на локали и апрувну.

@@ -27,4 +27,8 @@ export default class Config {
* Workspaces config-file path
*/
public static workspacesConfigPath: string = process.env.CONFIG_FILE || '';
/**
* Cron schedule
Copy link
Member

Choose a reason for hiding this comment

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

непонятное описание

import ServerService from '../services/server';

/**
* Statuses controller
Copy link
Member

Choose a reason for hiding this comment

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

непонятное описание

Copy link
Member

Choose a reason for hiding this comment

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

все еще

* Update statuses and send to logged users
*/
public static async updateStatuses(): Promise<void> {
const users = app.context.transport.clients.find(client => !!client).toArray();
Copy link
Member

Choose a reason for hiding this comment

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

зачем этот find?

Copy link
Contributor

Choose a reason for hiding this comment

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

Чтобы вернуть всех клиентов

Copy link
Member

Choose a reason for hiding this comment

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

чтобы вернуть всех, не нужно делать поиск по массиву


for (const workspaceAggregation of workspaceAggregations) {
const serverServicesStatuses: ServiceStatus = {} as ServiceStatus;
const serviceList = workspaceAggregation.servicesList[0].projectName.flat(Infinity);
Copy link
Member

Choose a reason for hiding this comment

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

magic number

Copy link
Contributor

@fabled228 fabled228 Feb 25, 2021

Choose a reason for hiding this comment

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

image

это лог workspaceAggregation

Copy link
Member

Choose a reason for hiding this comment

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

все равно magic number

if (ws) {
const workspaceAggregations = await WorkspacesService.aggregateServices(ws._id);

for (const workspaceAggregation of workspaceAggregations) {
Copy link
Member

Choose a reason for hiding this comment

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

непонятный код, нужно добавить комментариев

import ServiceStatus from '../database/models/serverServicesStatuses';

/**
* Server service
Copy link
Member

Choose a reason for hiding this comment

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

описание дублирует название

Comment on lines 81 to 83
public static async pingProjects(serviceType:string, workspaceAggregation: WorkspacesAggregation, serverServicesStatuses: ServiceStatus = {} as ServiceStatus): Promise<void> {
if (serviceType === 'nginx') {
const projectList = workspaceAggregation.servers.services.payload;
Copy link
Contributor

Choose a reason for hiding this comment

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

А нельзя просто принять projectList?

Comment on lines 86 to 88
for (const project of projectList) {
projects.push(project.serverName as string);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Может тут использовать map?

Copy link
Contributor

@kebyo kebyo left a comment

Choose a reason for hiding this comment

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

Мои сервера:
image

Service_statuses:
image

Почему-то в бд только 1 сервер

Comment on lines 80 to 82
public static async pingProjects(workspaceAggregation: WorkspacesAggregation, serverServicesStatuses: ServiceStatus = {} as ServiceStatus): Promise<void> {
const serviceType = workspaceAggregation.servers.services.type;

Copy link
Contributor

Choose a reason for hiding this comment

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

А может просто передать workspaceAgregation.servers?

Copy link
Member

@neSpecc neSpecc left a comment

Choose a reason for hiding this comment

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

Нужен рефакторинг

/**
* If type of service is nginx, server's projects are pinged
*/
await this.pingProjects(w.servers, serverServicesStatuses);
Copy link
Member

Choose a reason for hiding this comment

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

функция называется pingProjects, а принимает servers. Надо назватьpingServers, судя по всему

Copy link
Member

Choose a reason for hiding this comment

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

и зачем она принимает serverServicesStatuses? Она же сама должна пинговать, судя по названию.

Copy link
Member

Choose a reason for hiding this comment

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

и почему она принимает несколько серверов? Лучше сделать, чтобы принимала один конкретный хост и пинговала его

Copy link
Contributor

Choose a reason for hiding this comment

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

в servers тип + проекты какого-то сервера

* @param serverServicesStatuses - object with serverToken and statuses of server's projects
*/
public static async pingProjects(workspaceAggregationServer: IServer, serverServicesStatuses: ServiceStatus = {} as ServiceStatus): Promise<void> {
const serviceType = workspaceAggregationServer.services.type;
Copy link
Member

Choose a reason for hiding this comment

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

не понятно, что такое services.type. Type может быть у одного какого-то сервиса

Copy link
Member

Choose a reason for hiding this comment

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

и зачем сюда передавать какой-то не понятный объект workspaceAggregationServer . Надо передать один конкретный хост. Функция должна вернуть результат пинга. Обновление статуса в базе надо отсюда вынести. Ну или как-то соответствующим образом поменять название метода.

projectList.forEach((project) => {
projects.push(project.serverName as string);
});
const projectsStatuses = await StatusesController.checkingProjectsAvailability(projects);
Copy link
Member

Choose a reason for hiding this comment

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

что такое checkingProjectsAvailability ? почему нет глагола и почему передается сразу несколько проектов? пинговать же можно один какой-то хост.

* @param serviceStatuses - services' statuses and server token
*/
public static async updateServicesStatuses(serviceStatuses: IServiceStatus): Promise<mongoose.Document> {
const server = await ServiceStatus.findOne({ serverToken: serviceStatuses.serverToken });
Copy link
Member

Choose a reason for hiding this comment

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

почему в переменную server записывается не сервер, а какой-то статус?

/**
* Get workspace servers and their projects
*/
const workspaceAggregations: WorkspacesAggregation[] = await WorkspacesService.aggregateServices(workspace._id);
Copy link
Member

Choose a reason for hiding this comment

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

Эта агрегация портит структуру кода. Почему просто не взять workspace.servers циклом?

*
* @param projectList - list of project of payload of some service of some server
*/
public static async pingProjects(projectList: Record<string, unknown>[]): Promise<ProjectStatus[]> {
Copy link
Member

Choose a reason for hiding this comment

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

Почему Record<string, unknown>[] ? Нам же известна структура

Copy link
Contributor

Choose a reason for hiding this comment

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

потому что мы работаем с payload'ом сервисов, а у него такой тип
image

Copy link
Member

Choose a reason for hiding this comment

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

у payload должен быть Union Type с перечислением NginxPayload | DockerPayload | ...

*/
if (service.type == 'nginx') {
const projectsStatuses = await this.pingProjects(service.payload);
const serverProjectsStatuses: ServerProjectsStatuses = {} as ServerProjectsStatuses;
Copy link
Member

Choose a reason for hiding this comment

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

можно сразу написать

const serverProjectsStatuses: ServerProjectsStatuses = {
  projectsStatuses,
  serverToken: s.token
}

*
* @param serviceStatuses - services' statuses and server token
*/
public static async updateServicesStatuses(serviceStatuses: IServiceStatus): Promise<mongoose.Document> {
Copy link
Member

Choose a reason for hiding this comment

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

тут все еще бардак

updateServicesStatuses — статусы сервисов
serviceStatuses - статусы сервиса
ServiceStatus.updateOne - статус сервиса

а обновление происходит по serverToken, значит это обновление инфы о сервере, то есть в названии должно быть что-то типа updateServerPojectsStatuses

await this.add(serviceStatuses);
}

return ServiceStatus.updateOne({
Copy link
Member

Choose a reason for hiding this comment

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

а что такое ServiceStatus? они же лежат в одной таблице workspaces?

Copy link
Contributor

Choose a reason for hiding this comment

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

почему то по shift+fn+f6 не везде заменило имена, сейчас исправлю

Comment on lines 1 to 15
import ping from 'ping';
// eslint-disable-next-line @typescript-eslint/no-unused-vars-experimental,@typescript-eslint/no-unused-vars
import app from './../app';
import WorkspacesService from './../services/workspace';
import ServerProjectsStatuses, { ProjectStatus } from '../types/serverProjectsStatuses';
import IWorkspace from './../types/workspace';
// eslint-disable-next-line @typescript-eslint/no-unused-vars,@typescript-eslint/no-unused-vars-experimental
import Client from 'ctproto/build/src/server/client';
// eslint-disable-next-line @typescript-eslint/no-unused-vars,@typescript-eslint/no-unused-vars-experimental
import { DevopsToolboxAuthData } from '../types/api/responses/authorize';
// eslint-disable-next-line @typescript-eslint/no-unused-vars,@typescript-eslint/no-unused-vars-experimental
import { ApiOutgoingMessage, ApiResponse } from '../types';
import Server from '../services/server';
// eslint-disable-next-line @typescript-eslint/no-unused-vars,@typescript-eslint/no-unused-vars-experimental
import WorkspaceAggregation, { Server as IServer } from '../types/workspaceAggregation';
Copy link
Contributor

Choose a reason for hiding this comment

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

А eslint-disable нужны?

Comment on lines +104 to +111
// public static sendStatuses(statuses: ServerProjectsStatuses[], user: Client<DevopsToolboxAuthData, ApiResponse, ApiOutgoingMessage>): void {
// if (typeof user.authData.userToken === 'string') {
// app.context.transport
// .clients
// .find((client) => client.authData.userToken === user.authData.userToken)
// .send('statuses-updated', { statuses });
// }
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Если это тебе не нужно, то можно убрать это вообще

Copy link
Contributor

Choose a reason for hiding this comment

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

комменты уберу в некст реквесте

@@ -0,0 +1,28 @@
import * as mongoose from 'mongoose';
Copy link
Contributor

Choose a reason for hiding this comment

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

А почему не просто import mongoose from 'mongoose'?

Comment on lines 3 to 27

/**
* Interface for server
*/
export interface Server {
/**
* Server name
*/
name: string;

/**
* Integration token
* given on server creation
*/
token: string;

/**
* Information for SSH connection to server
*/
sshConnectionInfo: SSHConnectionInfo;

/**
* List of services running on the server
*/
services: Service[];
Copy link
Contributor

Choose a reason for hiding this comment

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

А зачем это надо? Нельзя ли просто import Server from './server'?

/**
* Get workspace servers and their projects
*/
const workspaceAggregations: WorkspaceAggregation[] = await WorkspacesService.aggregateServices(workspace._id);
Copy link
Member

Choose a reason for hiding this comment

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

объясни плз, зачем делать доп запросы в базу. Разве переменная workspaces не содержит серверы и сервисы?

*
* @param projectList - list of project of payload of some service of some server
*/
public static async pingProjects(projectList: Record<string, unknown>[]): Promise<ProjectStatus[]> {
Copy link
Member

Choose a reason for hiding this comment

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

у payload должен быть Union Type с перечислением NginxPayload | DockerPayload | ...

* @param serverProjectsStatuses - server projects' statuses and server token
*/
public static async updateServicesStatuses(serverProjectsStatuses: ServerProjectsStatuses): Promise<mongoose.Document> {
const server = await IServerProjectsStatuses.findOne({ serverToken: serverProjectsStatuses.serverToken });
Copy link
Member

Choose a reason for hiding this comment

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

Префикс I означает интерфейс. А у тебя тут какой то класс с таким префиксом

@@ -0,0 +1,33 @@
export interface Port {
Copy link
Contributor

Choose a reason for hiding this comment

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

docs

Comment on lines 22 to 26
export interface NginxService extends Service<NginxPayload> {
type: 'nginx';
}
export interface DockerService extends Service<DockerPayload> {
type: 'docker';
Copy link
Contributor

Choose a reason for hiding this comment

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

docs

for (const project of projectList) {
projectsStatuses.push(await StatusesController.checkProjectAvailability(project['serverName'] as string));
for (const payloadElement of payload) {
projectsStatuses.push(await StatusesController.checkProjectAvailability(payloadElement.serverName));
Copy link
Contributor

Choose a reason for hiding this comment

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

Я бы вынес это в переменную

projectsStatuses,
serverToken: s.token,
} as ServerProjectsStatuses;
if (service.type == 'nginx') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Я бы === поставил

Copy link
Contributor

Choose a reason for hiding this comment

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

В чем надобность?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ну тут особо разницы нет. Просто мы вроде везде используем ===

@fabled228 fabled228 requested review from neSpecc and ilyamore88 April 15, 2021 17:52
Comment on lines 9 to 20
/**
* Host for inside container
*/
host: string,
/**
* Port for inside container
*/
port: string,
/**
* Type of inside container
*/
type: string,
Copy link
Member

Choose a reason for hiding this comment

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

почему бы не вынести этот код в отдельный интерфейс, чтобы не дублировать его?

Comment on lines 46 to 48
listen: string,
serverName: string,
proxyPass: string,
Copy link
Member

Choose a reason for hiding this comment

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

доки?

Comment on lines 55 to 60
names: string,
containerId: string,
image: string,
created: Date,
status: string,
ports: Port[]
Copy link
Member

Choose a reason for hiding this comment

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

тоже доки

src/index.ts Outdated
* Ping availability of connected user's services
*/
cron.schedule(Config.pingSchedule, () => {
StatusesController.updateStatuses();
Copy link
Member

Choose a reason for hiding this comment

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

промис надо обработать

Comment on lines +2 to +16
// eslint-disable-next-line @typescript-eslint/no-unused-vars-experimental,@typescript-eslint/no-unused-vars
import app from './../app';
import WorkspacesService from './../services/workspace';
import ServerProjectsStatuses, { ProjectStatus } from '../types/serverProjectsStatuses';
import Workspace from './../types/workspace';
// eslint-disable-next-line @typescript-eslint/no-unused-vars,@typescript-eslint/no-unused-vars-experimental
import Client from 'ctproto/build/src/server/client';
// eslint-disable-next-line @typescript-eslint/no-unused-vars,@typescript-eslint/no-unused-vars-experimental
import { DevopsToolboxAuthData } from '../types/api/responses/authorize';
// eslint-disable-next-line @typescript-eslint/no-unused-vars,@typescript-eslint/no-unused-vars-experimental
import { ApiOutgoingMessage, ApiResponse } from '../types';
import Server from '../services/server';
import { NginxPayload } from '../types/servicePayload';

// eslint-disable-next-line @typescript-eslint/no-unused-vars,@typescript-eslint/no-unused-vars-experimental
Copy link
Member

Choose a reason for hiding this comment

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

что за беспредел

Copy link
Contributor

Choose a reason for hiding this comment

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

В следующем реквесте уберу..

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.

6 participants