Custom compatibility tags (#19415) #19610
Conversation
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢 |
|
🟢 |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢 |
PavelEfarinov
force-pushed
the
custom-compatibility-tags-19415
branch
from
5c9d741 to
ccb916a
Compare
|
⚪ Test history | Ya make output | Test bloat
🟢 |
|
⚪ Test history | Ya make output | Test bloat
🟢 |
| OUT_NOAUTO ydbd-last-stable ydbd-last-stable-name | ||
| ) | ||
|
|
||
| RUN_PROGRAM( | ||
| ydb/tests/library/compatibility/binaries/downloader stable-24-4/release/ydbd ydbd-prelast-stable 24-4 | ||
| ydb/tests/library/compatibility/binaries/downloader $COMPAT_INIT_REF/release/ydbd ydbd-prelast-stable $COMPAT_INIT_REF |
There was a problem hiding this comment.
Ну вот сейчас получается, что без задания COMPAT_INIT_REF оно не будет работать. Кроме того, дефолты задаются на уровне воркфлоу. Это приводит к тому, что чтобы запустить "дефолтное" поведение локально придется задавать эти переменные.
Мне кажется корректнее было бы дефолты хранить на уровне этого файла, а вот из воркфлоу это знание убрать. Например, можно поставить здесь условие "если не задано, то используй дефолт", а на уровне воркфлоу просто по дефолту не выставлять эти переременные (или выставлять в "")
There was a problem hiding this comment.
Из воркфлоу передачу этих параметров красиво не убрать, идея передавать пустую строку выглядит зыбко для поддержки в мейках. В ya.make добавил версии по-умолчанию для случаев, когда переменные не заданы вообще
There was a problem hiding this comment.
Ну вот все равно не очень красиво получается: у тебя инфа про дефолты в двух местах: в воркфлоу и в ямэйке
Может можно как-то убрать дублирование?
There was a problem hiding this comment.
Дублирование убрал, но на страничке воркфлоу сейчас не видно, на каких именно версиях мы запускаемся
|
⚪ Test history | Ya make output | Test bloat
🟢 |
|
⚪ Test history | Ya make output | Test bloat
🟢 |
| RUN_PROGRAM( | ||
| ydb/tests/library/compatibility/binaries/downloader stable-25-1/release/ydbd ydbd-last-stable 25-1 | ||
| ydb/tests/library/compatibility/binaries/downloader $COMPAT_INTER_REF/release/ydbd ydbd-last-stable $COMPAT_INTER_REF |
There was a problem hiding this comment.
А, сорри, что сразу не написал, мне кажется имя переменной, не очень хорошее, лучше сделать префикс YDB_ чтобы было понятно, что наша кастомщина, а не чья-то еще
То есть YDB_COMPAT_INTER_REF
Второй момент, у тебя используется терминология INIT/INTER а в других местах last/prelast. Как будто те же вещи называются разными именами, может было бы неплохо сделать, чтобы одинаково назывались
There was a problem hiding this comment.
Добавил префикс и переименовал last/prelast
| try: | ||
| result.append(int(elem)) | ||
| except ValueError: | ||
| result.append(float("NaN")) | ||
| if idx > 0: |
There was a problem hiding this comment.
Вот тут какое-то хакерство, имхо. Может лучше явно
- посмотреть первый элемент и если оно состоит из букв, то скипнуть
- написать комментарий зачем это нужно (мол обрабатываем случай "stable-25-1")
?
There was a problem hiding this comment.
Воообще, возможно более корректно было бы у самого ydbd спрашивать его версию, но тут требуется исследование и переделки
There was a problem hiding this comment.
Переписал, на явную обработку стейбла, добавил коммент
PavelEfarinov
force-pushed
the
custom-compatibility-tags-19415
branch
from
1c890db to
cae864b
Compare
|
⚪ Test history | Ya make output | Test bloat
🟢 |
|
⚪ Test history | Ya make output | Test bloat
🟢 |
PavelEfarinov
force-pushed
the
custom-compatibility-tags-19415
branch
from
cae864b to
39d2cee
Compare
|
⚪ Test history | Ya make output | Test bloat
🟢 |
|
⚪ Test history | Ya make output | Test bloat
🟢 |
PavelEfarinov
force-pushed
the
custom-compatibility-tags-19415
branch
from
39d2cee to
845eaf8
Compare
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
PavelEfarinov
force-pushed
the
custom-compatibility-tags-19415
branch
from
845eaf8 to
77976c5
Compare
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
PavelEfarinov
force-pushed
the
custom-compatibility-tags-19415
branch
from
77976c5 to
ba1996f
Compare
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
PavelEfarinov
force-pushed
the
custom-compatibility-tags-19415
branch
from
499b178 to
4332673
Compare
PavelEfarinov
force-pushed
the
custom-compatibility-tags-19415
branch
from
4332673 to
62e1d2e
Compare
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
ydb/tests/compatibility/ya.make
Outdated
| ydb/tests/library/compatibility/binaries | ||
| ) | ||
| IF(${YDB_COMPAT_TARGET_REF} == "current" OR NOT ${YDB_COMPAT_TARGET_REF}) | ||
| DEPENDS( | ||
| ydb/apps/ydb |
There was a problem hiding this comment.
ydb/apps/ydb -- это cli и кажется он должен быть всегда в зависимостях
То есть надо из под условия убрать
ydb/tests/compatibility/ya.make
Outdated
| IF(${YDB_COMPAT_TARGET_REF} == "current" OR NOT ${YDB_COMPAT_TARGET_REF}) | ||
| DEPENDS( | ||
| ydb/apps/ydb | ||
| ydb/apps/ydbd |
There was a problem hiding this comment.
Я еще вот такой ПР влил: #19869 кажется теперь правильно зависеть от
ydb/apps/ydbd
По другому)
There was a problem hiding this comment.
Добавил INCLUDE
| current_name = "current" | ||
| current_binary_version = (float("+inf"), ) | ||
|
|
||
| inter_stable_binary_path = yatest.common.binary_path("ydb/tests/library/compatibility/binaries/ydbd-inter-stable") |
There was a problem hiding this comment.
Надо бы тогда и в readme.md поправить терминологию, там вроде использовались last/prelast
There was a problem hiding this comment.
Нашел и исправил
| prelast_stable_name = open(yatest.common.binary_path("ydb/tests/library/compatibility/binaries/ydbd-prelast-stable-name")).read().strip() | ||
| prelast_stable_version = string_version_to_tuple(prelast_stable_name) | ||
| try: | ||
| current_binary_path = yatest.common.binary_path("ydb/tests/library/compatibility/binaries/ydbd-target-stable") |
There was a problem hiding this comment.
Мне кажется, что по хорошему тут не должно быть этого try-except, то есть обвязка должна гарантировать, что
ydb/tests/library/compatibility/binaries/ydbd-target-stable
всегда есть
Этого можно достичь, если ya.make в случае current будет просто копироать собранный ydbd по этому пути
There was a problem hiding this comment.
Но это не принципиально, имхо
There was a problem hiding this comment.
Избавился, теперь downloader формирует бинари
| prelast_stable_name = open(yatest.common.binary_path("ydb/tests/library/compatibility/binaries/ydbd-prelast-stable-name")).read().strip() | ||
| prelast_stable_version = string_version_to_tuple(prelast_stable_name) | ||
| try: | ||
| current_binary_path = yatest.common.binary_path("ydb/tests/library/compatibility/binaries/ydbd-target-stable") |
There was a problem hiding this comment.
Мне кажется слово -stable в названии файла лишнее. Это же необязательно стэйбл, какую семантику оно несет, может уберем?
There was a problem hiding this comment.
И в названии переменных тоже
Added tags param to yamake file of compatibility downloader. Added tag inoput to workflow Refs: #19415
Removed default stable branches input in compatibility workflow Ref: #19498
Added default compatibility version to ya.make Renamed old variables Refs: #19498
Added ref dependent run_name for compatibility workflow
PavelEfarinov
force-pushed
the
custom-compatibility-tags-19415
branch
from
f38505c to
d123b30
Compare
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
PavelEfarinov
force-pushed
the
custom-compatibility-tags-19415
branch
from
d123b30 to
8085ffb
Compare
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
PavelEfarinov
force-pushed
the
custom-compatibility-tags-19415
branch
from
8085ffb to
9e64fd6
Compare
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
Changelog category
Description for reviewers
Added new fields in regression compatibility workflow. Disabled default stable branches
-Successful builds
New workflow settings: