-
Notifications
You must be signed in to change notification settings - Fork 70
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
Исправление ошибки публикации по вложенным папкам в /build/out/allure #141
base: develop
Are you sure you want to change the base?
Conversation
Обзор измененийWalkthroughВ этом обновлении представлены незначительные изменения в нескольких файлах проекта Jenkins библиотеки. Модификации включают улучшение обработки путей, обновление логирования и комментирование некоторых операций. Изменения затрагивают утилиты работы с файлами, шаги публикации отчетов и обработку путей в различных компонентах библиотеки. Changes
Poem
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/ru/pulsar/jenkins/library/utils/FileUtils.groovy (1)
38-43
: Уточнить необходимость двойного вызова.toString()
и несколькихreplaceAll
Вместо двух подряд вызововreplaceAll('\\\\\\\\', '/')
иreplaceAll('\\\\', '/')
можно рассмотреть объединение этих выражений или заменить на один вызов, удаляющий все обратные слэши. Кроме того, повторный вызов.toString()
послеreplaceAll
может быть избыточным.src/ru/pulsar/jenkins/library/steps/Yaxunit.groovy (1)
89-90
: Рассмотреть удаление или восстановление закомментированного кода
Закомментированный вызов методаcopyTo
может впоследствии понадобиться, либо стоит его удалить во избежание лишнего «мусора» в коде. Если планируете вернуть этот участок, уточните и протестируйте, чтобы избежать ошибки формирования отчета Allure.src/ru/pulsar/jenkins/library/steps/PublishAllure.groovy (4)
52-54
: Логирование количества подкаталогов
Дополнительные логи упрощают диагностику проблемы с генерацией отчётов. Желательно уточнить, действительно ли нужно фиксировать общее число директорий и не обойти ли это проблему, когда реально нужны файлы в глубине вложенных папок.
68-70
: Закомментированные строки для отладки
Закомментированные вызовы указывают на прежние попытки напрямую использоватьgetLocalPath
при формировании пути. Если код уже не нужен, лучше окончательно удалить для поддержания чистоты кода.
73-76
: Проверка пустого списка результатов
Логирование и добавление пути к Allure при пустом списке кажется разумным способом избежать проблем с публикацией. Убедитесь, что при наличии множества подкаталогов данная логика не даст дублирование.
90-93
: МетодreplaceBackslashesWithSlashes
Простая реализация, обеспечивающая нормализацию слэшей. Можно дополнить проверкой, если путь уже содержит прямые слэши, чтобы избежать лишних операций, если критична производительность.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
LICENSE.md
(1 hunks)src/ru/pulsar/jenkins/library/steps/PublishAllure.groovy
(3 hunks)src/ru/pulsar/jenkins/library/steps/Yaxunit.groovy
(1 hunks)src/ru/pulsar/jenkins/library/utils/FileUtils.groovy
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- LICENSE.md
🔇 Additional comments (7)
src/ru/pulsar/jenkins/library/utils/FileUtils.groovy (2)
44-47
: Обратить внимание на случаи множественных “../”
Текущая проверка убирает только один префикс “../”. Если в результирующем пути могут оказаться более одного уровня “../”, это может привести к некорректному итогу. Стоит проверить, требуется ли устранение только одной вложенности или всех таких сегментов.
48-48
: Возврат итогового пути
Завершающая строка сreturn str
выглядит корректно. Убедитесь, что обработка ведущих “../” и замена слэшей полностью решает задачу формирования пути для всех сценариев.src/ru/pulsar/jenkins/library/steps/PublishAllure.groovy (5)
23-23
: Сообщение об отсутствии инициализации конфигурации
Вывод информативного журнала для случая, когдаconfig
не определён, выглядит корректно. При этом можно рассмотреть логгирование с более высоким уровнем серьёзности (например,Logger.error
), если данное состояние является критическим в контексте сборки.
46-46
: Анализ отсутствия результатов Allure
Сообщение «Отсутствуют результаты allure для публикации» даёт хорошее понимание состояния сборки. Убедитесь, что это ситуация не прерывает сборку, если нужно продолжать другие шаги.
55-58
: Формирование basePath и вывод в лог
ИспользованиеreplaceBackslashesWithSlashes
для формированияbasePath
помогает унифицировать путь. Убедитесь, что при работе на разных ОС логика не нарушается (например, при специфических путях в Windows/Unix).
60-63
: Локальный путь для текущей директории
ПолучениеpathCurrent
иpathdir
через FileUtils и вывод в лог выглядят понятно. Желательно проверить, корректно ли учитывается ситуация, когдаfilePath
уже содержит прямые слэши и не требует дополнительной нормализации.
64-67
: ВычислениеrezultPath
и добавление в результаты
Логика использованияgetRelativePath(pathdir, basePath)
добавляет гибкости при работе с вложенными папками. Проверьте, что итоговый относительный путь корректен даже при сложном уровне вложенности.
private static String getRelativePath(String absolutePath, String basePath) { | ||
def normalizedAbsolutePath = new File(absolutePath).canonicalPath | ||
def normalizedBasePath = new File(basePath).canonicalPath | ||
|
||
def relativePath = normalizedAbsolutePath.replaceFirst(normalizedBasePath, '') | ||
|
||
// Убираем начальный '/' если он есть | ||
if (relativePath.startsWith('/')) { | ||
relativePath = relativePath.substring(1) | ||
} | ||
|
||
return relativePath | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Метод getRelativePath
и риски совпадения путей
Расчёт относительного пути путём замены normalizedBasePath
на пустую строку подходит для большинства случаев. Однако если часть normalizedBasePath
может встречаться в середине normalizedAbsolutePath
, это вызовет ложное срабатывание. Рекомендуется проверить, что путь действительно начинается с normalizedBasePath
.
|
||
private static String getRelativePath(String absolutePath, String basePath) { | ||
def normalizedAbsolutePath = new File(absolutePath).canonicalPath | ||
def normalizedBasePath = new File(basePath).canonicalPath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Класс File использовать нужно использовать очень аккуратно, так как он разрешается на мастере, а не на агенте. Лучше работать через FilePath. Ну и соответственно вынести эту логику в FileUtils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Класс File использовать нужно использовать очень аккуратно, так как он разрешается на мастере, а не на агенте. Лучше работать через FilePath. Ну и соответственно вынести эту логику в FileUtils
Груви не знаю. Через нейросеть эту функцию получил и применил, чтобы закрыть проблему.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тут не чистый груви, а с ограничениями дженкинса и его распределенной модели выполнения.
.toString() | ||
.replaceAll('\\\\\\\\', '/') | ||
.replaceAll('\\\\', '/') | ||
.toString() | ||
|
||
if (str.startsWith("../")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это сильно меняет логику работы этого метода
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
На сейчас эта функция работает не очень корректно. И явно это проблема раз на выходе может впереди быть "../". Функцию надо значительно модифицировать, т.к. она ключевая.
getLocalPath(c:\j\workspace\ins-lib_projects_ci_kyporos_main\build\out\allure\bdd) = "../c:/j/workspace/ins-lib_projects_ci_kyporos_main/build/out/allure/bdd"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Меня смущает то, что никто из других пользователей с такой ошибкой не сталкивался. Может проблема в вашей версии дженкинса? Какая у вас?
Спасибо за пулл-реквест, но можно чуть подробнее расписать суть изменений? Как было и как стало. Особенно смущает изменения по вырезанию ../, это не выглядит корректным. |
Было: getLocalPath(c:\j\workspace\ins-lib_projects_ci_kyporos_main\build\out\allure\bdd) = "../c:/j/workspace/ins-lib_projects_ci_kyporos_main/build/out/allure/bdd" Груви не знаю и как мог поправил. Теперь все работает корректно. У меня включены yaxunit и BDD все работает и результат с двух разных вложенных папок в /build/out/allure отрабатывают. Мастер Jenkins на убунто а нода агента 1С на винде |
Может быть вот тут как раз проблема. Текущая реализация через File работает, на линуксе виндовые слэши могут некорректно разбираться. Из-за этого и неправильное поведение в релятивизации папки. |
И как такое решить? Дженкинс может быть и на винде и на линуксе |
@nixel2007 вот тоже проблема из ишузов, скорей всего эта же #129 |
Корректно обрабатывать пути через FilePath |
Не, там нет проблемы с путями как таковыми. Да и сетап по осям другой. |
Не корректно определяет путь по FileUtils.getLocalPath(filePath) при подготовке данных для allure.