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

Aerospike fixtures support #168

Merged
merged 9 commits into from
Jul 8, 2022
Merged

Aerospike fixtures support #168

merged 9 commits into from
Jul 8, 2022

Conversation

vitkarpenko
Copy link
Contributor

Заливка данных в аэроспайк: #111. Чекеры будут отдельным ПР, хочется ещё подумать над форматом

@vitkarpenko
Copy link
Contributor Author

vitkarpenko commented Jun 27, 2022

Хотел немного порефакторить по ходу, но что-то выглядит так что переборщил и ПР раздулся. Почти все содержательные изменения в первом коммите (a799709), но если всё равно неудобно смотреть могу разбить на два ПРа.

@vitkarpenko vitkarpenko requested a review from fetinin June 27, 2022 11:05
README.md Outdated
@@ -594,6 +605,55 @@ tables:
- created_at: $eval(NOW())
```

### Aerospike
Для хранилища Aerospike также поддерживается заливка тестовых данных. Для этого важно не забыть при запуске gonkey как CLI-приложение использовать флаг `-db-type aerospike`, а при использовании в качестве библиотеки в конфигурации раннера: `DbType: fixtures.Postgres`.
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.

Спасибо, поправил :)

README-ru.md Outdated
@@ -592,6 +603,54 @@ tables:
- created_at: $eval(NOW())
```

### Aerospike
Для хранилища Aerospike также поддерживается заливка тестовых данных. Для этого важно не забыть при запуске gonkey как CLI-приложение использовать флаг `-db-type aerospike`, а при использовании в качестве библиотеки в конфигурации раннера: `DbType: fixtures.Postgres`.
Copy link
Contributor

Choose a reason for hiding this comment

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

а тут не fixtures.Aerospike вместо fixtures.Postgres?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Исправил

@@ -1,13 +1,16 @@
.PHONY: setup
setup:
setup: teardown
Copy link
Contributor

@fetinin fetinin Jun 28, 2022

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.

Просто пока тестил по привычке сделал при запуске сначала очистку чтобы было удобнее :) Думаешь стоит убрать?

Copy link
Contributor

Choose a reason for hiding this comment

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

договорились teardown запускать в конце тестов, а тут оставить на всякий случай если teardown не отработал из-за зафейлившегося теста

@docker-compose -f docker-compose.yaml up --build -d
@sleep 2
Copy link
Contributor

@fetinin fetinin Jun 28, 2022

Choose a reason for hiding this comment

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

чтобы не костылять со sleep, как вариант можно настроить у нужного сервиса хелсчек и в compose добавить флаг --wait чтобы дождаться пока сервис пройдет хелсчек и начнёт отвечать на запросы
docker/compose#8777

Copy link
Contributor Author

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.

обсудили, договорились попробовать всё же хелсчек для питон-сервиса настроить

@@ -4,6 +4,7 @@ go 1.14

require (
github.com/Masterminds/sprig/v3 v3.2.2
github.com/aerospike/aerospike-client-go/v5 v5.8.0
Copy link
Contributor

Choose a reason for hiding this comment

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

кажется не оч. хорошо тащить клиент аэроспайка и его зависимости всем кто использует gonkey, но не использует аэроспайк. Можно было бы наши заливаторы фикстур разнести по отдельным модулям, как было сделано с чекером openapi вот здесь
https://github.com/lamoda/gonkey/tree/160a29cfac68d18e01e7e1ef9293f5722e93ac5e/checker/addons/openapi2_compliance

Пока сходу правда тяжело сказать на сколько большим фронтом работ это обернётся

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Хмм, вообще хороший поинт, спасибо. Но что-то меня смущает, что в ридми написано о -spec <...> флаге запуска, в то время как на самом деле такого флага нет. Да и сам openapi_compliance чекер кажется не используется нигде? Кажется при запуске через testing его можно передать, но в ридми это насколько я вижу никак не отражено 🤔 Странно.

Copy link
Contributor

@fetinin fetinin Jul 4, 2022

Choose a reason for hiding this comment

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

обсудили варианты, в текущей реализации gonkey не получится вынести загрузчик в отдельный модуль не ломая обратную совместимость. решили не ломать и оставит как есть

@vitkarpenko vitkarpenko requested a review from fetinin July 4, 2022 15:21
@vitkarpenko vitkarpenko merged commit 79f277a into lamoda:master Jul 8, 2022
@github-actions
Copy link

github-actions bot commented Jul 8, 2022

🚀 PR was released in v1.18.2 🚀

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

Successfully merging this pull request may close these issues.

2 participants