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.

Sign up for GitHub

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

Jump to bottom

Core dumps repository #1

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Core dumps repository #1

wants to merge 3 commits into from

Conversation

srjchsv
Copy link
Contributor

@srjchsv srjchsv commented Sep 17, 2022

No description provided.

@srjchsv srjchsv force-pushed the CoreDumpsRepository branch from d1bc507 to 2fd163d Compare September 17, 2022 13:36
@srgchrksv
feat: CoreDumpsRepository implementation and test
e044620 
feat: CoreDumpsRepository implementation and test
@srjchsv srjchsv force-pushed the CoreDumpsRepository branch from e3118de to e044620 Compare September 17, 2022 13:53
VladimirBalun
VladimirBalun requested changes Sep 18, 2022
View reviewed changes
server/internal/app/configuration.go Outdated
ServerAddress = os.Getenv("CRASHER_SERVER_ADDRESS")

ServerAddress = os.Getenv("CRASHER_SERVER_ADDRESS")
CtxTimeout = os.Getenv("CRASHER_DATABASE_TIMEOUTgit")
Copy link
Contributor

Choose a reason for hiding this comment

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

git - что-то лишнее)
И давай название переменной сделаем DatabaseTimeout

Copy link
Contributor Author

@srjchsv srjchsv Sep 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Right)
CRASHER_DATABASE_TIMEOUT or DATABASE_TIMEOUT?

server/internal/app/entities/core_dump.go Outdated
status CoreDumpStatus
data string
timestamp time.Time
ID primitive.ObjectID
Copy link
Contributor

Choose a reason for hiding this comment

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

entity - ничего не должен знать о БД, в будущем замена БД должна быть бесшовной

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what type for ID then? int?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed ID to string type as its in the method parameter and API scheme.
How to set a unique ID when creating a dump? We are missing a string ID setter...

server/internal/app/entities/core_dump.go Outdated
Extensions []Extensions
}

type Extensions struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Extension давай только, а вот выше давай так оставим Extensions []Extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

allright

server/internal/app/entities/core_dump.go Outdated
Status CoreDumpStatus
Data string
Timestamp time.Time
Extensions []Extensions
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 Author

Choose a reason for hiding this comment

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

allright

server/internal/app/entities/query_fields_mongo.go Outdated
@@ -0,0 +1,43 @@
package entities
Copy link
Contributor

Choose a reason for hiding this comment

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

Внутри entities не должно быть никакого mongo - стоит это вынести к mongodb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

server/internal/app/repositories/mongodb_repository/core_dumps_repository_test.go Show resolved Hide resolved
server/internal/app/repositories/mongodb_repository/core_dumps_repository_test.go Outdated
}
}

func Test_DeleteCoreDump(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Для DeleteAll еще нужен

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

server/internal/app/repositories/mongodb_repository/core_dumps_repository_test.go Outdated Show resolved Hide resolved
server/internal/app/repositories/mongodb_repository/core_dumps_repository_test.go Outdated Show resolved Hide resolved
server/internal/app/repositories/mongodb_repository/core_dumps_repository_test.go Show resolved Hide resolved
@VladimirBalun VladimirBalun added the enhancement New feature or request label Sep 18, 2022
@srjchsv srjchsv force-pushed the CoreDumpsRepository branch from 6cf1acd to ae80ef3 Compare September 18, 2022 20:58
VladimirBalun
VladimirBalun requested changes Sep 19, 2022
View reviewed changes
server/internal/app/configuration.go Outdated
ServerAddress = os.Getenv("CRASHER_SERVER_ADDRESS")

ServerAddress = os.Getenv("CRASHER_SERVER_ADDRESS")
CtxTimeout = parseEnvOrDefaultValue("CRASHER_DATABASE_TIMEOUT", "5")
Copy link
Contributor

Choose a reason for hiding this comment

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

Название переменной, как и говорил, давай сделаем DatabaseTimeout

Copy link
Contributor

Choose a reason for hiding this comment

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

Не надо здесь парсить - парсить будем в main-e

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

server/internal/app/entities/core_dump.go Outdated
}

func (c *CoreDump) Timestamp() time.Time {
return c.timestamp
func (c *CoreDump) GetExtension(index int) *Extension {
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 Author

Choose a reason for hiding this comment

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

allright, deleted it

server/internal/app/entities/core_dump.go Outdated
c.Timestamp = timestamp
}

func (c *CoreDump) SetExtensions(key, value string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Если метод называется SetExtensions он должен устанавливать все расширения, если говорим о добавление - это должно быть AddExtension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

allright

server/internal/app/entities/core_dump.go Outdated
Extensions []Extension
}

type Extension struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Давай назовем CoreDumpExtension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

allright

server/internal/app/entities/core_dump.go Outdated
return &c.Extensions[index]
}

func (c *CoreDump) GetExtensions() *[]Extension {
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 Author

Choose a reason for hiding this comment

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

No pointer, allright.

server/internal/app/repositories/mongodb_repository/core_dumps_repository_test.go Show resolved Hide resolved
server/internal/app/repositories/mongodb_repository/core_dumps_repository_test.go
error: errors.New("error enter dump id"),
},
}
for _, test := range tests {
Copy link
Contributor

Choose a reason for hiding this comment

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

Забыл

server/internal/app/repositories/mongodb_repository/core_dumps_repository_test.go Outdated
rand.Seed(time.Now().Unix())
}

func generateRandomSliceOfCoreDumps(quantity int) []entities.CoreDump {
Copy link
Contributor

Choose a reason for hiding this comment

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

Забыл

server/internal/app/repositories/mongodb_repository/mongo_options/query_fields_mongo_test.go Outdated
res := filter["appinfo.name"]

require.Equal(t, "app", res)
require.Equal(t, true, len(filter) > 0)
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.

И для тестов ниже

server/internal/app/repositories/mongodb_repository/mongo_options/query_fields_mongo_test.go Outdated
setter(filter, options)
res := filter["appinfo.name"]

require.Equal(t, "app", res)
Copy link
Contributor

Choose a reason for hiding this comment

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

Здесь нужны ассерты, а не require

Copy link
Contributor

Choose a reason for hiding this comment

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

И для тестов ниже

@srjchsv srjchsv force-pushed the CoreDumpsRepository branch from 634a3ef to ed81e3f Compare September 19, 2022 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VladimirBalun VladimirBalun Awaiting requested review from VladimirBalun

Assignees

@srjchsv srjchsv

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@srjchsv @VladimirBalun @srgchrksv