Поддержка 1cedtcli #133
Поддержка 1cedtcli #133
Conversation
WalkthroughВ данном пулл-запросе внесены изменения в несколько классов, связанных с преобразованием форматов EDT и Designer. Основное внимание уделяется внедрению класса Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
src/ru/pulsar/jenkins/library/utils/VersionParser.groovy (2)
46-46: Добавьте документацию методаРекомендуется добавить JavaDoc с описанием:
- Назначения метода
- Параметров
- Возвращаемого значения
- Исключений
Предлагаю добавить:
+ /** + * Сравнивает две версии в формате x.y.z + * @param thisVersion Первая версия для сравнения + * @param thatVersion Вторая версия для сравнения + * @return 1 если thisVersion больше, -1 если меньше, 0 если версии равны + * @throws IllegalArgumentException если версии null, пустые или имеют неверный формат + */ static int compare(String thisVersion, String thatVersion) {
46-63: Добавьте модульные тестыДля обеспечения надежности метода
compareнеобходимо добавить тесты, покрывающие:
- Сравнение версий разной длины
- Краевые случаи
- Обработку исключений
Хотите, чтобы я помог составить модульные тесты для этого метода?
src/ru/pulsar/jenkins/library/steps/DesignerToEdtFormatTransformation.groovy (2)
46-47: Замечание: Уточнить версии в сообщениях логированияВ логах используется версия "2024.1.X", тогда как сравнение производится с версией "2024". Рекомендуется скорректировать сообщения для согласованности с фактическими условиями проверки.
55-56: Замечание: Уточнить версии в сообщениях логированияАналогично предыдущему комментарию, следует привести версию в сообщении к "2024" для一致ия с условием проверки.
src/ru/pulsar/jenkins/library/steps/EdtValidate.groovy (3)
59-61: Уточните сравнение версий и сообщения логовВ условии используется сравнение с версией "2024", а в сообщении лога упоминается версия "2024.1.X". Для согласованности рекомендуется использовать одинаковую версию в сравнении и сообщениях. Если сравнение происходит с "2024", то в логах стоит указать "2024", чтобы избежать путаницы.
63-66: Унифицируйте методы выполнения командВ первом блоке вы используете
steps.ringCommand(ringCommand), а во втором блоке —steps.cmd(edtcliCommand, false, true). Рекомендуется унифицировать способ выполнения команд для повышения читаемости и поддержки кода. Возможно, стоит создать общий метод, который принимает команду и соответственно вызывает нужный метод.
59-77: Рассмотрите возможность рефакторинга условных блоковКод внутри блоков
ifиelseимеет схожую структуру с небольшими отличиями в командах и используемых методах. Рассмотрите возможность вынесения общих частей кода в отдельные методы или упрощения логики для уменьшения дублирования и повышения читаемости.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/ru/pulsar/jenkins/library/steps/DesignerToEdtFormatTransformation.groovy(2 hunks)src/ru/pulsar/jenkins/library/steps/EdtToDesignerFormatTransformation.groovy(3 hunks)src/ru/pulsar/jenkins/library/steps/EdtValidate.groovy(2 hunks)src/ru/pulsar/jenkins/library/utils/VersionParser.groovy(1 hunks)test/unit/groovy/ru/pulsar/jenkins/library/utils/VersionParserTest.java(1 hunks)
🔇 Additional comments (5)
test/unit/groovy/ru/pulsar/jenkins/library/utils/VersionParserTest.java (2)
88-101: Тест корректно проверяет сравнение версий!
Тест хорошо структурирован и проверяет важный сценарий сравнения версий, где первая версия меньше второй.
103-116:
Проверьте корректность логики сравнения версий разной длины.
Текущая реализация теста предполагает, что версии "2024.2.4" и "2024.2" равны (результат сравнения = 0). Это может быть неожиданным поведением, так как:
- Версия "2024.2" может интерпретироваться как "2024.2.0"
- В этом случае "2024.2.4" должна быть больше "2024.2.0"
Пожалуйста, подтвердите, что это ожидаемое поведение для бизнес-логики.
Предлагаю добавить дополнительные тест-кейсы:
+ @Test
+ void testVersionComparisonWithImplicitZero() {
+ // given
+ String thisVersion = "2024.2.0";
+ String thatVersion = "2024.2";
+
+ // when
+ int result = VersionParser.compare(thisVersion, thatVersion);
+
+ // then
+ assertThat(result).isEqualTo(0);
+ }src/ru/pulsar/jenkins/library/steps/EdtToDesignerFormatTransformation.groovy (1)
12-12: LGTM: Добавлен импорт VersionParser
Корректное добавление импорта для новой функциональности сравнения версий.
src/ru/pulsar/jenkins/library/steps/DesignerToEdtFormatTransformation.groovy (1)
10-10: Импорт класса VersionParser подтверждён
Импорт необходим для сравнения версий EDT.
src/ru/pulsar/jenkins/library/steps/EdtValidate.groovy (1)
74-75: Проверьте необходимость вывода stdOut в лог
В блоке с 1cedtcli вы выводите стандартный вывод команды в лог с помощью Logger.println(stdOut). Убедитесь, что этот вывод необходим и не приведет к перегрузке логов или раскрытию чувствительной информации. Если вывод очень объемный или содержит небезопасные данные, возможно, стоит его ограничить или удалить.
src/ru/pulsar/jenkins/library/steps/EdtToDesignerFormatTransformation.groovy
Outdated
Show resolved
Hide resolved
src/ru/pulsar/jenkins/library/steps/EdtToDesignerFormatTransformation.groovy
Outdated
Show resolved
Hide resolved
src/ru/pulsar/jenkins/library/steps/DesignerToEdtFormatTransformation.groovy
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (6)
src/ru/pulsar/jenkins/library/edt/EdtCliEngineFactory.groovy (1)
5-6: Добавьте документацию к классу.Рекомендуется добавить JavaDoc с описанием назначения фабрики и поддерживаемых движков конвертации.
+/** + * Фабрика для создания движков конвертации EDT/Designer форматов. + * Выбирает подходящую реализацию в зависимости от версии EDT: + * - До версии 2024: использует ring + * - Версия 2024 и выше: использует 1cedtcli + */ class EdtCliEngineFactory {src/ru/pulsar/jenkins/library/steps/DesignerToEdtFormatTransformation.groovy (1)
31-32: Рекомендуется добавить обработку ошибокТекущая реализация корректно делегирует работу фабрике движков, что улучшает поддержку кода. Однако стоит добавить обработку потенциальных ошибок при получении движка.
Предлагаю обернуть код в try-catch:
- def engine = EdtCliEngineFactory.getEngine(config.edtVersion) - engine.designerToEdtTransform(steps, config) + try { + def engine = EdtCliEngineFactory.getEngine(config.edtVersion) + engine.designerToEdtTransform(steps, config) + } catch (Exception e) { + Logger.println("Ошибка при выполнении трансформации: ${e.message}") + throw e + }src/ru/pulsar/jenkins/library/steps/EdtValidate.groovy (2)
54-56: Отличная реализация фабричного паттерна!Использование
EdtCliEngineFactoryзначительно упрощает код и инкапсулирует логику выбора между ring и 1cedtcli. Это делает код более поддерживаемым и соответствующим принципу единой ответственности.Рекомендуется добавить документацию к методу
edtValidate, описывающую особенности работы с разными версиями EDT. Это поможет другим разработчикам быстрее разобраться в логике работы.
Требуется обновление документации для EDT 2024+
Анализ показал:
- Логика сравнения версий в
EdtCliEngineFactoryреализована корректно - используется строгое сравнение с версией 2024- В README отсутствует информация о требованиях к PATH для EDT 2024+ и использовании 1cedtcli
- Отсутствует документация в каталоге docs/
Необходимо:
- Добавить в README.md информацию о новых требованиях к окружению для работы с EDT 2024+
- Описать различия в использовании ring и 1cedtcli
🔗 Analysis chain
Line range hint
1-63: Необходима проверка обработки версий EDTРеализация выглядит корректной, но требуется проверка следующих аспектов:
- Корректность определения версии EDT для выбора между ring и 1cedtcli
- Обновление README с информацией о требованиях к PATH для EDT 2024+
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Проверяем наличие обновлений в README rg -i "1cedtcli|path" README.md # Проверяем логику сравнения версий ast-grep --pattern 'getEngine($_)'Length of output: 2442
Script:
#!/bin/bash # Поиск реализации фабрики и логики сравнения версий EDT ast-grep --pattern 'class EdtCliEngineFactory { $$$ }' rg -A 10 "getEngine" --type groovyLength of output: 4074
Script:
#!/bin/bash # Проверяем реализацию конвертеров ast-grep --pattern 'class RingConverter { $$$ }' ast-grep --pattern 'class NativeEdtCliConverter { $$$ }' # Проверяем документацию по EDT rg -i "edt.*version|1cedtcli" docs/Length of output: 197
src/ru/pulsar/jenkins/library/edt/RingConverter.groovy (2)
65-65: Согласовать использованиеgetRemote()послеgetFilePath()В других местах после вызова
FileUtils.getFilePathиспользуется методgetRemote(). Рекомендуется добавитьgetRemote(), чтобы получить строковый путь и обеспечить согласованность.-def workspaceDir = FileUtils.getFilePath("$env.WORKSPACE/$DesignerToEdtFormatTransformation.WORKSPACE") +def workspaceDir = FileUtils.getFilePath("$env.WORKSPACE/$DesignerToEdtFormatTransformation.WORKSPACE").getRemote()
88-90: ИспользоватьFileUtils.getFilePathдля формирования путейВ методе
edtValidateпеременныеworkspaceLocationиresultFileформируются без использованияFileUtils.getFilePath, тогда как в других методах для получения путей используетсяFileUtils.getFilePath. Рекомендуется использоватьFileUtils.getFilePathдля консистентности и корректной обработки путей.-def workspaceLocation = "$env.WORKSPACE/$DesignerToEdtFormatTransformation.WORKSPACE" +def workspaceLocation = FileUtils.getFilePath("$env.WORKSPACE/$DesignerToEdtFormatTransformation.WORKSPACE").getRemote() -def resultFile = "$env.WORKSPACE/$EdtValidate.RESULT_FILE" +def resultFile = FileUtils.getFilePath("$env.WORKSPACE/$EdtValidate.RESULT_FILE").getRemote()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/ru/pulsar/jenkins/library/edt/EdtCliEngineFactory.groovy(1 hunks)src/ru/pulsar/jenkins/library/edt/IEdtCliEngine.groovy(1 hunks)src/ru/pulsar/jenkins/library/edt/NativeEdtCliConverter.groovy(1 hunks)src/ru/pulsar/jenkins/library/edt/RingConverter.groovy(1 hunks)src/ru/pulsar/jenkins/library/steps/DesignerToEdtFormatTransformation.groovy(2 hunks)src/ru/pulsar/jenkins/library/steps/EdtToDesignerFormatTransformation.groovy(2 hunks)src/ru/pulsar/jenkins/library/steps/EdtValidate.groovy(2 hunks)
🔇 Additional comments (7)
src/ru/pulsar/jenkins/library/edt/EdtCliEngineFactory.groovy (2)
1-4: LGTM!
Корректный импорт ModuleDescriptor для работы с версиями.
1-18: Проверьте использование новой фабрики в существующем коде.
Убедитесь, что все места, где создаются конвертеры, обновлены для использования фабрики.
✅ Verification successful
Let me search for any usage of converters to ensure we haven't missed anything.
Фабрика корректно используется во всех местах создания конвертеров
Проверка показала, что:
- Прямое создание конвертеров (
RingConverterиNativeEdtCliConverter) происходит только внутри самой фабрики - Фабрика
EdtCliEngineFactoryправильно используется во всех местах, где требуется конвертер:DesignerToEdtFormatTransformation.groovyEdtToDesignerFormatTransformation.groovyEdtValidate.groovy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Поиск прямого создания конвертеров в обход фабрики
echo "Поиск создания RingConverter:"
rg "new RingConverter\(\)" --type groovy
echo -e "\nПоиск создания NativeEdtCliConverter:"
rg "new NativeEdtCliConverter\(\)" --type groovy
Length of output: 461
Script:
#!/bin/bash
# Поиск использования конвертеров через методы и интерфейсы
echo "Поиск использования RingConverter:"
rg "RingConverter" --type groovy -A 2
echo -e "\nПоиск использования NativeEdtCliConverter:"
rg "NativeEdtCliConverter" --type groovy -A 2
echo -e "\nПоиск использования EdtCliEngineFactory:"
rg "EdtCliEngineFactory" --type groovy -A 2
Length of output: 3859
src/ru/pulsar/jenkins/library/edt/IEdtCliEngine.groovy (2)
1-5: Корректная организация пакетов и импортов.
Структура пакетов и импорты соответствуют стандартам Java/Groovy.
6-11: Хорошо структурированный интерфейс с четким разделением ответственности.
Интерфейс корректно определяет основные операции для работы с форматами EDT и Designer. Методы имеют понятные названия и логичную группировку параметров.
Давайте проверим использование интерфейса в кодовой базе:
✅ Verification successful
Интерфейс корректно реализован и активно используется в проекте
Анализ показал:
- Интерфейс реализован двумя классами:
RingConverterиNativeEdtCliConverter - Все методы корректно имплементированы в обоих классах
- Обработка ошибок присутствует через механизм
catchError - Интерфейс активно используется в различных частях системы (pipeline1C, EdtValidate, ResultsTransformer)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Описание: Поиск классов, реализующих интерфейс IEdtCliEngine
ast-grep --pattern 'class $_ implements IEdtCliEngine {
$$$
}'
Length of output: 66
Script:
#!/bin/bash
# Поиск реализаций интерфейса с учетом возможных вариантов синтаксиса Groovy
rg -l "implements.*IEdtCliEngine|class.*implements.*IEdtCliEngine"
# Поиск использования методов интерфейса
rg "edtToDesignerTransformConfiguration|edtToDesignerTransformExtensions|designerToEdtTransform|edtValidate"
# Поиск импортов интерфейса
rg "import.*IEdtCliEngine"
Length of output: 4632
Script:
#!/bin/bash
# Проверим реализации интерфейса в найденных классах
rg -A 10 "class.*RingConverter|class.*NativeEdtCliConverter" src/ru/pulsar/jenkins/library/edt/
# Проверим обработку ошибок в реализациях
rg "throw|catch|error" src/ru/pulsar/jenkins/library/edt/RingConverter.groovy src/ru/pulsar/jenkins/library/edt/NativeEdtCliConverter.groovy
Length of output: 2847
src/ru/pulsar/jenkins/library/steps/DesignerToEdtFormatTransformation.groovy (1)
3-3: LGTM: Корректное добавление импорта
Добавление импорта EdtCliEngineFactory соответствует новой архитектуре с поддержкой различных CLI-движков.
src/ru/pulsar/jenkins/library/steps/EdtValidate.groovy (1)
3-3: Корректное добавление импорта!
Импорт EdtCliEngineFactory логично вписывается в общую структуру импортов и необходим для реализации фабричного паттерна.
src/ru/pulsar/jenkins/library/steps/EdtToDesignerFormatTransformation.groovy (1)
42-48: Применение паттерна Стратегия улучшает расширяемость
Замена прямых вызовов на использование EdtCliEngineFactory и engine соответствует предыдущему предложению о применении паттерна Стратегия. Это повышает модульность и упрощает добавление новых вариантов преобразования в будущем.
src/ru/pulsar/jenkins/library/steps/EdtToDesignerFormatTransformation.groovy
Show resolved
Hide resolved
src/ru/pulsar/jenkins/library/steps/EdtToDesignerFormatTransformation.groovy
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
src/ru/pulsar/jenkins/library/edt/NativeEdtCliConverter.groovy (2)
80-90: Добавьте проверку существования рабочих каталогов и файлов перед валидацией.Переменные
workspaceLocationиresultFileиспользуются без проверки их существования. Рекомендуется добавить проверки перед вызовом команды валидации, чтобы избежать возможных ошибок.Примените следующий дифф для исправления:
String workspaceLocation = "$env.WORKSPACE/$DesignerToEdtFormatTransformation.WORKSPACE" def resultFile = "$env.WORKSPACE/$EdtValidate.RESULT_FILE" + if (!steps.fileExists(workspaceLocation)) { + Logger.println("Рабочее пространство не существует: $workspaceLocation") + return + } + + if (!steps.fileExists(resultFile)) { + Logger.println("Файл результата не существует: $resultFile") + return + } def edtcliCommand = "1cedtcli -data \"$workspaceLocation\" -command validate --file \"$resultFile\" $projectList"
15-90: Рассмотрите возможность рефакторинга для устранения дублирования кода.Методы содержат повторяющийся код для формирования команд и обработки ошибок. Рекомендуется вынести общую логику в отдельные методы для повышения читаемости и поддержки кода.
Предложение:
- Создать приватный метод для выполнения команд с обработкой ошибок.
- Вынести общую логику формирования команд
1cedtcli.
…enkins-lib into feature/1cedtcli
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/ru/pulsar/jenkins/library/edt/EdtCliEngineFactory.groovy (1)
9-30: Улучшить документацию формата версииВ документации метода
getEngineуказан формат версии какYYYY.X.Z, YYYY.X или YYYY, но стоит добавить:
- Допустимые значения для X и Z
- Примеры корректных версий
- Описание поведения при сравнении версий разного формата (например, "2024" vs "2024.1")
README.md (1)
26-26: Предлагаю расширить документацию по использованию 1cedtcliТекущее изменение корректно описывает новое требование. Предлагаю дополнить документацию следующей информацией:
- Добавить ссылку на документацию по установке и настройке 1cedtcli
- Описать возможность использования 1cedtcli для версий EDT 2023.* (опционально)
- Указать, как проверить корректность установки 1cedtcli в PATH
-1. При использовании EDT версии 2024.1.0 и выше вместо ring используется 1cedtcli, который должен быть прописан в PATH на агенте. +1. При использовании EDT версии 2024.1.0 и выше вместо ring используется 1cedtcli: + * Утилита должна быть прописана в PATH на агенте + * Инструкция по установке: <ссылка на документацию> + * Для проверки корректности установки выполните команду `1cedtcli --version` + * Для EDT версии 2023.* можно принудительно использовать 1cedtcli, установив параметр `useEdtcli: true` в конфигурации
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
README.md(1 hunks)src/ru/pulsar/jenkins/library/edt/EdtCliEngineFactory.groovy(1 hunks)src/ru/pulsar/jenkins/library/edt/NativeEdtCliConverter.groovy(1 hunks)src/ru/pulsar/jenkins/library/edt/RingConverter.groovy(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ru/pulsar/jenkins/library/edt/NativeEdtCliConverter.groovy
- src/ru/pulsar/jenkins/library/edt/RingConverter.groovy
🔇 Additional comments (2)
src/ru/pulsar/jenkins/library/edt/EdtCliEngineFactory.groovy (2)
1-8: Структура класса и константы реализованы корректно!
Правильное решение использовать константу EDT_CLI_MIN_VERSION вместо магического числа "2024". Парсинг версии через ModuleDescriptor.Version обеспечивает надежную обработку версий.
32-39: Проверить поведение сравнения версий
Текущая реализация использует оператор >= для сравнения версий. Необходимо убедиться, что сравнение корректно работает для всех возможных форматов версий.
Fix #100
Добавил простой метод сравнения версий в
VersionParser, он может в будущем пригодиться не только для EDT. В зависимости от результата вызова этого метода в соответствующих местах вызывается либо ring, либо 1cedtcli
Что хочу доделать:
isEdtcliRequiredОстался не учтенным кейс, когда у пользователя EDT 2023.* и он хочет использовать 1cedtcli вместо ring. Можно добавить еще один параметр в конфиг, но надо ли?
UPD: для больших конфигураций также пригодится указание параметра
-vmargs, как это сделано в обертке над командами ring