Всем привет!
В предыдущем посте я упомянул про защиту от DOS аттак в коде. Раскрою тему.
Для начала стоит различать DDOS и DOS - (Distribted) Denial of Service.
Первый - это когда злоумышленник долбит миллионами запросов в секунду. Такое не выдержит ни один сервис, не поможет даже k8s, т.к. ресурсы кластера не резиновые - https://kubernetes.io/docs/setup/best-practices/cluster-large/ да и подымаются новые ноды не мгновенно. Следовательно, от DDOS должна защищать сетевая инфраструктура, прикладной разработчик тут ничего сделать не может.
Другое дело DOS - RPS на порядки меньше, эксплуатируются уязвимости в коде. Вопрос - откуда злоумышленники про них узнают?
Во-первых они могут действовать наугад, во-вторых - всегда могут быть болтливые сотрудники, а главное - защита типа "об этом никто никогда не узнает" - плохая защиты.
Суть всех уязвимостией при DOS одна - поднять на сервере столько потоков одновременно, чтобы закончилась память или загрузка процессора ушла под 100%
Итак, как можно улучшить код для защиты от DOS.
1) проводить нагрузочное тестирование (НТ). НТ позволяет точно определеить сколько нужно серверов, чтобы держать расчетную и пиковую нагрузку. Пиковую нагрузку можно взять как расчетная умножить на два. Утечки памяти, неоптимальный код - все это с большой вероятностью можно увидеть на НТ
2) нет бесконечным и большим таймаутам. Если смежник упал, а у нас бесконечный таймаут - потоки и память кончатся быстро. Что касается больших таймаутов - это минуты, или таймауты необоснованные с точки зрения бизнес-задачи.
2) таймауты должны быть согласованы. Если мы обрабатываем запрос с таймутом 5 секунд, он синхронный, а вызываем смежника с таймутом 10 секунд - мы зря тратим его и свои ресурсы. Согласование может быть ручным, либо можно слать, например в заголовках, свой таймаут смежнику, чтобы он не ждал зря.
3) использовать circuit breaker, он же предохранитель, он же техперерыв. Если известно, что смежная система прилегла - не надо ее добивать и тратить на это свои ресурсы. Берем данные из кэша если это возможно или возвращаем клиенту ошибку. Принцип fail fast. Стоит отметить, что настройку таймаутов, предохранителя, и числа повторов можно делать либо в коде, либо отдать на откуп Istio или аналогичной системе если мы в облаке. Что лучше - это отдельная тема
4) защищаться от уязвимостей типа Injection. Суть их в том, что злоумышленник передает в параметрах входящего запроса что-то, что приводит к нелинейному потреблению ресуросов или тяжелым запросам в БД. Примеры первого вида DTD схемы https://habr.com/ru/post/170333/, регулярки - https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS, второго - SQL Injection со сложными JOIN. Решение: валидация параметров, регулярно сканировать библиотеки на наличие уязвимостей, регулярно обновляться в части хотфиксов
5) логика сервиса не должна линейно зависеть от числа входных параметров либо число параметров должно ограничено. Чем-то похоже на предыдущий пункт, но тут приложение спроектировано криво, поэтому никакие уязвимости не нужны)
6) использовать пулы потоков. Во многих случаях они уже используются - обработка входящих веб-запросов, JDBC запросы к БД. Но есть потоки, которые создаем мы сами. Если на каждый входной запрос мы будем создавать дополнительно хотя бы +1 поток, то это примерно удвоит потребление ресурсов. А если больше одного... Пул потоков защищает от такой ситуации
7) не забывать закрывать ресурсы - файлы, коннекты к БД. Все что java.io.Closeable. И делать это правильно - try with resources. В отличие от памяти в куче ресурсы никто за вас не закроет. А они жрут память и часто ограничены: максимальное число открытых файлов в Linux, максимальное число запросов, которое может обрабатывать СУБД
8) не использовать тяжелые JOIN и GROUP BY запросы к БД. Создавать индексы, смотреть план выполнения запроса. Об этом должен позаботиться ваш DBA, но, увы, не всегда он есть
9) не использовать излишне сильные уровни блокировки в БД, не использовать блокировки файлов без явной необходимости
#code_quality #security #patterns
В предыдущем посте я упомянул про защиту от DOS аттак в коде. Раскрою тему.
Для начала стоит различать DDOS и DOS - (Distribted) Denial of Service.
Первый - это когда злоумышленник долбит миллионами запросов в секунду. Такое не выдержит ни один сервис, не поможет даже k8s, т.к. ресурсы кластера не резиновые - https://kubernetes.io/docs/setup/best-practices/cluster-large/ да и подымаются новые ноды не мгновенно. Следовательно, от DDOS должна защищать сетевая инфраструктура, прикладной разработчик тут ничего сделать не может.
Другое дело DOS - RPS на порядки меньше, эксплуатируются уязвимости в коде. Вопрос - откуда злоумышленники про них узнают?
Во-первых они могут действовать наугад, во-вторых - всегда могут быть болтливые сотрудники, а главное - защита типа "об этом никто никогда не узнает" - плохая защиты.
Суть всех уязвимостией при DOS одна - поднять на сервере столько потоков одновременно, чтобы закончилась память или загрузка процессора ушла под 100%
Итак, как можно улучшить код для защиты от DOS.
1) проводить нагрузочное тестирование (НТ). НТ позволяет точно определеить сколько нужно серверов, чтобы держать расчетную и пиковую нагрузку. Пиковую нагрузку можно взять как расчетная умножить на два. Утечки памяти, неоптимальный код - все это с большой вероятностью можно увидеть на НТ
2) нет бесконечным и большим таймаутам. Если смежник упал, а у нас бесконечный таймаут - потоки и память кончатся быстро. Что касается больших таймаутов - это минуты, или таймауты необоснованные с точки зрения бизнес-задачи.
2) таймауты должны быть согласованы. Если мы обрабатываем запрос с таймутом 5 секунд, он синхронный, а вызываем смежника с таймутом 10 секунд - мы зря тратим его и свои ресурсы. Согласование может быть ручным, либо можно слать, например в заголовках, свой таймаут смежнику, чтобы он не ждал зря.
3) использовать circuit breaker, он же предохранитель, он же техперерыв. Если известно, что смежная система прилегла - не надо ее добивать и тратить на это свои ресурсы. Берем данные из кэша если это возможно или возвращаем клиенту ошибку. Принцип fail fast. Стоит отметить, что настройку таймаутов, предохранителя, и числа повторов можно делать либо в коде, либо отдать на откуп Istio или аналогичной системе если мы в облаке. Что лучше - это отдельная тема
4) защищаться от уязвимостей типа Injection. Суть их в том, что злоумышленник передает в параметрах входящего запроса что-то, что приводит к нелинейному потреблению ресуросов или тяжелым запросам в БД. Примеры первого вида DTD схемы https://habr.com/ru/post/170333/, регулярки - https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS, второго - SQL Injection со сложными JOIN. Решение: валидация параметров, регулярно сканировать библиотеки на наличие уязвимостей, регулярно обновляться в части хотфиксов
5) логика сервиса не должна линейно зависеть от числа входных параметров либо число параметров должно ограничено. Чем-то похоже на предыдущий пункт, но тут приложение спроектировано криво, поэтому никакие уязвимости не нужны)
6) использовать пулы потоков. Во многих случаях они уже используются - обработка входящих веб-запросов, JDBC запросы к БД. Но есть потоки, которые создаем мы сами. Если на каждый входной запрос мы будем создавать дополнительно хотя бы +1 поток, то это примерно удвоит потребление ресурсов. А если больше одного... Пул потоков защищает от такой ситуации
7) не забывать закрывать ресурсы - файлы, коннекты к БД. Все что java.io.Closeable. И делать это правильно - try with resources. В отличие от памяти в куче ресурсы никто за вас не закроет. А они жрут память и часто ограничены: максимальное число открытых файлов в Linux, максимальное число запросов, которое может обрабатывать СУБД
8) не использовать тяжелые JOIN и GROUP BY запросы к БД. Создавать индексы, смотреть план выполнения запроса. Об этом должен позаботиться ваш DBA, но, увы, не всегда он есть
9) не использовать излишне сильные уровни блокировки в БД, не использовать блокировки файлов без явной необходимости
#code_quality #security #patterns
Kubernetes
Considerations for large clusters
A cluster is a set of nodes (physical or virtual machines) running Kubernetes agents, managed by the control plane. Kubernetes v1.33 supports clusters with up to 5,000 nodes. More specifically, Kubernetes is designed to accommodate configurations that meet…
Всем привет!
Пару слов про на мой взгляд достаточно важную, но часто недооцененную штуку, как комментарии к commit-ам в git. На самом деле к любой Version Control System, но у нас же на 95+% git)))
Что должно быть:
1) комментарии обязательны
2) в теле комментария должен быть тикет, по которому ведутся работы. В начале текста комментария. В идеале - один тикет. Если правка одной строки исправляет несколько багов\закрывает несколько задач - то лучше закрыть лишние тикеты как дубликаты. Если не хочется заводить тикет на каждый чих - можно завести и использовать тикет типа "мелкие правки"
3) любой комментарий должен быть содержательным, т.е. описывать суть изменений. Лучше не привязваться к именам файлов\классов, т.к. они меняются при рефакторинге, а оперировать понятиями предметной области
4) по длине рекомендую сильно не увлекаться, ограничиваться одной или несколькими фразами. Заодно и сommit-ы будут более атомарными, проще будет revert-ть.
5) формат сommit должен быть единообразным, можно даже контролировать это на сервере. Т.к. по моему опыту без контроля сложно придерживаться единообразия, периодически проскакивают неформатные комментарии)
6) конечная цель комментариев - чтобы список commit-ов нес полезную информацию и его можно было использовать как changelist сервиса
7) следствие из предыдущего пункта - можно и нужно использовать amend для слияния нескольких commit-ов в один. Единственное исключение - если ваш git server синхронизируется, в другую подсеть например, и синхронизация ломается при amend. Опять же при односторонней синхронизации проблему решить можно.
8) следующий уровень - перед созданием Pull request\Merge request можно отрефакторить список commit-ов - переименовать, слить несколько из середины. IDEA позволяет все это сделать. Также можно хардкорно, через коммандную строку с помощью rebase - https://habr.com/ru/post/201922/ С одной стороны так мы становится на путь перфекционизма, но помнить о такой возможности надо)
9) Описание формата commit должно быть в readme.md проекта
Отдельно хочу сказать про язык комментариев. Есть мнение о необходимости английского - я с ним не согласен. По желанию команды. Все-таки не все в совершенстве владеют английским, как среди текущих разработчиков сервиса, так и среди будущих. А читаемость прежде всего!) Да, в английском нет родов у глаголов, поэтому с ним проще. В русском лучшим вариантом кажется использовать стадательный залог - "сделано то-то". Добавление рода или числа - сделал\сделали\сделала - кажется лишней информацией.
Также отдельная тема - нужны ли какие-то специальные обозначения в commit message. Например, алиас для тип изменений: фича, багфикс, рефакторинг, конфиги, тесты. Или указание модуля, где менялся код. Считаю, что не обязательно, но может быть полезно.
#git #code_review
Пару слов про на мой взгляд достаточно важную, но часто недооцененную штуку, как комментарии к commit-ам в git. На самом деле к любой Version Control System, но у нас же на 95+% git)))
Что должно быть:
1) комментарии обязательны
2) в теле комментария должен быть тикет, по которому ведутся работы. В начале текста комментария. В идеале - один тикет. Если правка одной строки исправляет несколько багов\закрывает несколько задач - то лучше закрыть лишние тикеты как дубликаты. Если не хочется заводить тикет на каждый чих - можно завести и использовать тикет типа "мелкие правки"
3) любой комментарий должен быть содержательным, т.е. описывать суть изменений. Лучше не привязваться к именам файлов\классов, т.к. они меняются при рефакторинге, а оперировать понятиями предметной области
4) по длине рекомендую сильно не увлекаться, ограничиваться одной или несколькими фразами. Заодно и сommit-ы будут более атомарными, проще будет revert-ть.
5) формат сommit должен быть единообразным, можно даже контролировать это на сервере. Т.к. по моему опыту без контроля сложно придерживаться единообразия, периодически проскакивают неформатные комментарии)
6) конечная цель комментариев - чтобы список commit-ов нес полезную информацию и его можно было использовать как changelist сервиса
7) следствие из предыдущего пункта - можно и нужно использовать amend для слияния нескольких commit-ов в один. Единственное исключение - если ваш git server синхронизируется, в другую подсеть например, и синхронизация ломается при amend. Опять же при односторонней синхронизации проблему решить можно.
8) следующий уровень - перед созданием Pull request\Merge request можно отрефакторить список commit-ов - переименовать, слить несколько из середины. IDEA позволяет все это сделать. Также можно хардкорно, через коммандную строку с помощью rebase - https://habr.com/ru/post/201922/ С одной стороны так мы становится на путь перфекционизма, но помнить о такой возможности надо)
9) Описание формата commit должно быть в readme.md проекта
Отдельно хочу сказать про язык комментариев. Есть мнение о необходимости английского - я с ним не согласен. По желанию команды. Все-таки не все в совершенстве владеют английским, как среди текущих разработчиков сервиса, так и среди будущих. А читаемость прежде всего!) Да, в английском нет родов у глаголов, поэтому с ним проще. В русском лучшим вариантом кажется использовать стадательный залог - "сделано то-то". Добавление рода или числа - сделал\сделали\сделала - кажется лишней информацией.
Также отдельная тема - нужны ли какие-то специальные обозначения в commit message. Например, алиас для тип изменений: фича, багфикс, рефакторинг, конфиги, тесты. Или указание модуля, где менялся код. Считаю, что не обязательно, но может быть полезно.
#git #code_review
Хабр
Изменение коммитов в Git
Это пост для тех, кто начинает работу с Git. Все, что здесь написано по частям можно найти в многочисленных простынях о Git на Хабре. Но я подумал, что неплохо было бы иметь отдельный предельно...
Всем привет!
Сегодня хотел бы поговорить про код-ревью.
Для начала - что оно нам дает?
1) единообразие кодовой базы. Люди приходят и уходят, а код остается) Новичок в команде должен во-первых быстро разобраться в проекте, а во-вторых - писать код в едином стиле. Я говорю не только про код-стайл, но и про наименования, разделение по модулям и слоям, переиспользование кода и многое другое
2) понятный, "чистый" код в проекте. Тут можно снова сказать про важность быстрого вхождения в проект новых людей. Важная ремарка - чтобы код был простым и понятным ревьювер должен сознательно ставить себе такую цель.
3) общее владение кодом в команде. Если код посмотрел ревьювер и сделал это качественно, а не формально - этот код теперь знают два человека. Легче вносить какие-то правки, проводить рефакторинг.
4) альтернативный взгляд на решение задачи. Не всегда предложенное решение будет правильным, и нужно все переделывать. Но узнать о других вариантах и обсудить к примеру будущий рефакторинг точно будет полезно. Отдельно хочется отменить, что иногда лучше прорабатывать решение до написания кода, чтобы избежать потерь времени на код-ревью и переделывание. Примеры: новичок в коданде или просто сложная задача.
5) обмен знаниями о проекте, библиотеках, утилитах и даже языке программирования. Причем узнать что-то новое может и автор кода, и ревьювер.
6) повторяющиеся ошибки\"холиварные" вопросы на код-ревью должны обсуждаться и фиксироваться. И т.об. они являются источником для новых правил в статическом анализаторе кода и в правилах работы над проектом
И как следствие вышесказанного получаем повышение качества кода.
P.S. Далее расскажу про рекомендации для автора и ревьювера, а также про кросс-командное ревью.
#code_review
Сегодня хотел бы поговорить про код-ревью.
Для начала - что оно нам дает?
1) единообразие кодовой базы. Люди приходят и уходят, а код остается) Новичок в команде должен во-первых быстро разобраться в проекте, а во-вторых - писать код в едином стиле. Я говорю не только про код-стайл, но и про наименования, разделение по модулям и слоям, переиспользование кода и многое другое
2) понятный, "чистый" код в проекте. Тут можно снова сказать про важность быстрого вхождения в проект новых людей. Важная ремарка - чтобы код был простым и понятным ревьювер должен сознательно ставить себе такую цель.
3) общее владение кодом в команде. Если код посмотрел ревьювер и сделал это качественно, а не формально - этот код теперь знают два человека. Легче вносить какие-то правки, проводить рефакторинг.
4) альтернативный взгляд на решение задачи. Не всегда предложенное решение будет правильным, и нужно все переделывать. Но узнать о других вариантах и обсудить к примеру будущий рефакторинг точно будет полезно. Отдельно хочется отменить, что иногда лучше прорабатывать решение до написания кода, чтобы избежать потерь времени на код-ревью и переделывание. Примеры: новичок в коданде или просто сложная задача.
5) обмен знаниями о проекте, библиотеках, утилитах и даже языке программирования. Причем узнать что-то новое может и автор кода, и ревьювер.
6) повторяющиеся ошибки\"холиварные" вопросы на код-ревью должны обсуждаться и фиксироваться. И т.об. они являются источником для новых правил в статическом анализаторе кода и в правилах работы над проектом
И как следствие вышесказанного получаем повышение качества кода.
P.S. Далее расскажу про рекомендации для автора и ревьювера, а также про кросс-командное ревью.
#code_review
8) Ревью кода - не менее сложная задача, чем его написание. Я бы даже сказал более сложная. А чем больше кода - тем сложнее в него вникнуть. Поэтому лучше договориться на берегу о размерах PR в команде и строго требовать соблюдения этого правила. Если договоренностей не было - один раз стоит "помучаться", потом поговорить с автором, но начиная со 2-го отклонять такие PR. Иначе есть риск, что большая часть правок будет пролистана, ведь часто даже не понятно, где ключевые изменения. А в этом случае теряется смысл ревью кода. Еще альтернатива - спросить у автора куда смотреть в первую очередь. Но это все костыли, лучше придерживаться правила по размеру. Я бы его ограничил в 100 строк кода.
9) Я надеюсь, что у вас подключен автоматический prcheck - проверка PR. Если это так - не нужно комментировать то, что уже прокомментировал бот SonarQube. О том, какие замечания SonarQube правим также нужно договорится на берегу и включить в Quality Gate.
Да, о настройках DevOps для код ревью сделаю отдельный пост.
10) Типовые ошибки на код-ревью лучше проговорить со всей командой и вынести в отдельный документ или в SonarQube. Это сэкономит время в будущем.
11) Не забывайте пользоваться галкой игнорировать различие в пробелах, если ваша система для проведения ревью поддерживает такую фичу.
12) Нужно строго соблюдать scope задачи. Как бы ни хотелось поправить еще один, найденный на код-ревью баг - нельзя, это затягивает процесс. Начали с исправления бага, а закончили архитектурным рефакторингом всего сервиса - да, возможно, но не в рамках одного PR) А у нас есть Lead Time и бэклог
13) Если ревью зашло в тупик - подключайте вашего техлида, тимлида, любого авторитетного разработчика. Возможно нужен взгляд со стороны.
14) В абсолютном большинстве PR в целом код хороший. Если это не так - стоит подумать там ли вы работаете) Если вы видите крутое решение - стоит это отметить отдельным комментарием. Если просто хороший код, но с замечаниями - в первом замечании стоит отметить, что "в целом все хорошо, но..."
15) если в PR несколько однотипных ошибок, не стоит оставлять замечания на всех. Это захламляет PR. Лучше отменить одну, максимум две. А далее можно посмотреть - найдет ли автор остальные)
#code_review
9) Я надеюсь, что у вас подключен автоматический prcheck - проверка PR. Если это так - не нужно комментировать то, что уже прокомментировал бот SonarQube. О том, какие замечания SonarQube правим также нужно договорится на берегу и включить в Quality Gate.
Да, о настройках DevOps для код ревью сделаю отдельный пост.
10) Типовые ошибки на код-ревью лучше проговорить со всей командой и вынести в отдельный документ или в SonarQube. Это сэкономит время в будущем.
11) Не забывайте пользоваться галкой игнорировать различие в пробелах, если ваша система для проведения ревью поддерживает такую фичу.
12) Нужно строго соблюдать scope задачи. Как бы ни хотелось поправить еще один, найденный на код-ревью баг - нельзя, это затягивает процесс. Начали с исправления бага, а закончили архитектурным рефакторингом всего сервиса - да, возможно, но не в рамках одного PR) А у нас есть Lead Time и бэклог
13) Если ревью зашло в тупик - подключайте вашего техлида, тимлида, любого авторитетного разработчика. Возможно нужен взгляд со стороны.
14) В абсолютном большинстве PR в целом код хороший. Если это не так - стоит подумать там ли вы работаете) Если вы видите крутое решение - стоит это отметить отдельным комментарием. Если просто хороший код, но с замечаниями - в первом замечании стоит отметить, что "в целом все хорошо, но..."
15) если в PR несколько однотипных ошибок, не стоит оставлять замечания на всех. Это захламляет PR. Лучше отменить одну, максимум две. А далее можно посмотреть - найдет ли автор остальные)
#code_review
Всем привет!
Теперь перейдем к рекомендациям для авторов Pull Request (PR), они же Merge Request.
1) не нужно тратить время ревьювера на то, что могут сделать роботы) Я про чистку import и форматирование кода. Рекомендую поставить эти действия на автовыполнение при сохранении в IDEA. Т.об. ревьювер увидит более корректный код, и ему будет проще найти серьезные ошибки, ради которых мы и проводим ревью кода.
2) не стоит отправлять на сервер код, для которого не проверена компилируемость и прохождение тестов. Да, я надеюсь ваш DevOps пайплайн это проверяет, но даже в таком случае это лишняя нагрузка на инфраструктуру, а также лишний цикл ошибка-исправление-запуск CI и замедление скорости попадания кода на тестовую среду и в конечном итоге на ПРОМ.
3) также рекомендую подключить SonarLint в IDEA и чинить баги SonarQube локально. Причины те же, что в предыдущем пункте
4) как я уже писал - ревью кода тяжелая работа. С увеличением размера кода сложность этой работы увеличивается, причем нелинейно. Поэтому декомпозируйте задачу, уменьшайте каждого размер PR. Если изменений по одной декомпозированной фиче все равно достаточно много - разбивайте PR на несколько commit. Рекомендуемый размер PR - не более 100 строк.
5) а чтобы облегчить понимание сути изменений в PR - делайте содержательные комментарии, как именно - я уже описывал в https://t.me/javaKotlinDevOps/81 Если кроме информации в отдельных commit и в тикете, по которому сделан PR, есть еще какая-то высокоуровневая логика, понимание которой важно для ревью - напишите об этом в шапке PR или отдельным комментарием к PR.
6) если ревьювер "пропал" (это плохой, но вполне реальный кейс) - постарайтесь достучаться до него по всем возможным каналам и понять причину отсутствия реакции на PR. Если оперативное ревью невозможно - найдите другого ревьювера.
7) снова о психологии) замечания к PR - это не замечания к вам лично, это замечания к вашему коду. Поэтому относится к замечаниям надо спокойно, возможно с помощью ревьювера вы узнаете что-то новое о сервисе, фреймворке, платформе или даже языке программирования. Ошибки делают все, это нормально. Главное не доводить их до ПРОМа)
8) Если копнуть глубже - ваш код на самом деле не ваш, это код команды. Вся команда отвечает за фичу. Т.е. вы с ревьювером в одной лодке, ваша общая цель - быстро вывести качественную фичу на ПРОМ.
9) как бы ни был велик соблазн пропихнуть в PR еще пару тройку мелких правок - ему нужно сопротивляться. По себе знаю, что это сложно) Но scope PR должен соблюдаться, это сильно упрощает ревью!
10) код-ревью - это диалог, а замечания - не приговор. Ревьювер может что-то не понять, не учесть или просто ошибаться. В этом случае нужно ему вежливо объяснить детали
11) ну и снова скажу что текст - ограниченный способ коммуникации. Чтобы избежать недопонимания - пообщайтесь с ревьювером лично если есть возможность, позвоните ему или поговорите по зуму
12) если PR сложный и\или работа над ним шла долго - по возможности гляньте его сами, перед тем как добавлять ревьюверов. Посмотрите именно как PR, чтобы увидеть что изменилось и что увидит ревьювер. Возможно, найдете какие-то простые баги сами или поймете, что нужно добавить комментарий
13) на замечания ревьювера лучше отвечать правками в коде, после которых они станут неактуальны, чем объяснять что-то в комментариях. Если идею сразу не понял ревьювер - очень может быть что ее не поймут люди, которые будут править этот код через год
14) и еще одна важная вещь: недостаточно просто поправить замечание в коде. Напишите что-то типа "сделано" в ответ на каждое замечание. Скажу по себе - ревьювер вам мысленно скажет спасибо!)
#code_review
Теперь перейдем к рекомендациям для авторов Pull Request (PR), они же Merge Request.
1) не нужно тратить время ревьювера на то, что могут сделать роботы) Я про чистку import и форматирование кода. Рекомендую поставить эти действия на автовыполнение при сохранении в IDEA. Т.об. ревьювер увидит более корректный код, и ему будет проще найти серьезные ошибки, ради которых мы и проводим ревью кода.
2) не стоит отправлять на сервер код, для которого не проверена компилируемость и прохождение тестов. Да, я надеюсь ваш DevOps пайплайн это проверяет, но даже в таком случае это лишняя нагрузка на инфраструктуру, а также лишний цикл ошибка-исправление-запуск CI и замедление скорости попадания кода на тестовую среду и в конечном итоге на ПРОМ.
3) также рекомендую подключить SonarLint в IDEA и чинить баги SonarQube локально. Причины те же, что в предыдущем пункте
4) как я уже писал - ревью кода тяжелая работа. С увеличением размера кода сложность этой работы увеличивается, причем нелинейно. Поэтому декомпозируйте задачу, уменьшайте каждого размер PR. Если изменений по одной декомпозированной фиче все равно достаточно много - разбивайте PR на несколько commit. Рекомендуемый размер PR - не более 100 строк.
5) а чтобы облегчить понимание сути изменений в PR - делайте содержательные комментарии, как именно - я уже описывал в https://t.me/javaKotlinDevOps/81 Если кроме информации в отдельных commit и в тикете, по которому сделан PR, есть еще какая-то высокоуровневая логика, понимание которой важно для ревью - напишите об этом в шапке PR или отдельным комментарием к PR.
6) если ревьювер "пропал" (это плохой, но вполне реальный кейс) - постарайтесь достучаться до него по всем возможным каналам и понять причину отсутствия реакции на PR. Если оперативное ревью невозможно - найдите другого ревьювера.
7) снова о психологии) замечания к PR - это не замечания к вам лично, это замечания к вашему коду. Поэтому относится к замечаниям надо спокойно, возможно с помощью ревьювера вы узнаете что-то новое о сервисе, фреймворке, платформе или даже языке программирования. Ошибки делают все, это нормально. Главное не доводить их до ПРОМа)
8) Если копнуть глубже - ваш код на самом деле не ваш, это код команды. Вся команда отвечает за фичу. Т.е. вы с ревьювером в одной лодке, ваша общая цель - быстро вывести качественную фичу на ПРОМ.
9) как бы ни был велик соблазн пропихнуть в PR еще пару тройку мелких правок - ему нужно сопротивляться. По себе знаю, что это сложно) Но scope PR должен соблюдаться, это сильно упрощает ревью!
10) код-ревью - это диалог, а замечания - не приговор. Ревьювер может что-то не понять, не учесть или просто ошибаться. В этом случае нужно ему вежливо объяснить детали
11) ну и снова скажу что текст - ограниченный способ коммуникации. Чтобы избежать недопонимания - пообщайтесь с ревьювером лично если есть возможность, позвоните ему или поговорите по зуму
12) если PR сложный и\или работа над ним шла долго - по возможности гляньте его сами, перед тем как добавлять ревьюверов. Посмотрите именно как PR, чтобы увидеть что изменилось и что увидит ревьювер. Возможно, найдете какие-то простые баги сами или поймете, что нужно добавить комментарий
13) на замечания ревьювера лучше отвечать правками в коде, после которых они станут неактуальны, чем объяснять что-то в комментариях. Если идею сразу не понял ревьювер - очень может быть что ее не поймут люди, которые будут править этот код через год
14) и еще одна важная вещь: недостаточно просто поправить замечание в коде. Напишите что-то типа "сделано" в ответ на каждое замечание. Скажу по себе - ревьювер вам мысленно скажет спасибо!)
#code_review
Telegram
(java || kotlin) && devOps
Всем привет!
Пару слов про на мой взгляд достаточно важную, но часто недооцененную штуку, как комментарии к commit-ам в git. На самом деле к любой Version Control System, но у нас же на 95+% git)))
Что должно быть:
1) комментарии обязательны
2) в теле комментария…
Пару слов про на мой взгляд достаточно важную, но часто недооцененную штуку, как комментарии к commit-ам в git. На самом деле к любой Version Control System, но у нас же на 95+% git)))
Что должно быть:
1) комментарии обязательны
2) в теле комментария…
Всем привет!
Поговорим про инфраструктуру и DevOps, не относящуюся напрямую к ревью кода, но сильно облегчающую его проведение.
1) обязательно должна быть автоматическая проверка Pull Request (PR) на компилируемость, прохождение тестов и статический анализ кода. Одно из распространенных названий этой процедуры - prcheck. Те ошибки, что могут найти роботы - должны искать роботы)
2) обязательно должна автоматически запускаться контрольная сборка с запуском тестов после вливания кода в master\develop\release. Так мы как можно раньше локализуем проблемы, которые могут возникнуть после слияния. Несмотря на наличие prcheck и ручное ревью после слияния вполне могут появится ошибки. Особенно важен этот и предыдущий пункт если у вас не микросервис и над кодом работают несколько команд. Т.к. в этом случае ошибка, попавшая в master, заблокирует работу всех команд.
3) рекомендую сделать документ с требованиями по коду (guideline), которые невозможно автоматизировать. То, что можно автоматизировать - описывать текстом не нужно, а следует добавить в статический анализатор. SonarQube, номер один на этом рынке, дает нам кучу правил из коробки, кроме того есть дополнительные плагины с правилами, например, https://github.com/spotbugs/sonar-findbugs, а также можно создавать свои правила. Правила нужно поддерживать его в актуальном состоянии и строго им следовать. Это может быть файл в формате Markdown (.md) в Bitbucket, тогда с ним тоже можно работать через PR, либо статья в формате wiki. Главное чтобы четко было понятно, где лежит последняя версия правил
4) также должны быть правила наименования веток, PR и формата commit
5) в случае монолита будет полезным механизм, определяющий какого рода код изменился и автоматически добавляющий в PR ответственных за данную функциональную область
6) как я уже говорил ранее может быть полезен отдельный чатик для призыва ревьюверов в PR) email уведомления тоже полезны, но почту читают реже, письма могут попасть в спам
#code_review
Поговорим про инфраструктуру и DevOps, не относящуюся напрямую к ревью кода, но сильно облегчающую его проведение.
1) обязательно должна быть автоматическая проверка Pull Request (PR) на компилируемость, прохождение тестов и статический анализ кода. Одно из распространенных названий этой процедуры - prcheck. Те ошибки, что могут найти роботы - должны искать роботы)
2) обязательно должна автоматически запускаться контрольная сборка с запуском тестов после вливания кода в master\develop\release. Так мы как можно раньше локализуем проблемы, которые могут возникнуть после слияния. Несмотря на наличие prcheck и ручное ревью после слияния вполне могут появится ошибки. Особенно важен этот и предыдущий пункт если у вас не микросервис и над кодом работают несколько команд. Т.к. в этом случае ошибка, попавшая в master, заблокирует работу всех команд.
3) рекомендую сделать документ с требованиями по коду (guideline), которые невозможно автоматизировать. То, что можно автоматизировать - описывать текстом не нужно, а следует добавить в статический анализатор. SonarQube, номер один на этом рынке, дает нам кучу правил из коробки, кроме того есть дополнительные плагины с правилами, например, https://github.com/spotbugs/sonar-findbugs, а также можно создавать свои правила. Правила нужно поддерживать его в актуальном состоянии и строго им следовать. Это может быть файл в формате Markdown (.md) в Bitbucket, тогда с ним тоже можно работать через PR, либо статья в формате wiki. Главное чтобы четко было понятно, где лежит последняя версия правил
4) также должны быть правила наименования веток, PR и формата commit
5) в случае монолита будет полезным механизм, определяющий какого рода код изменился и автоматически добавляющий в PR ответственных за данную функциональную область
6) как я уже говорил ранее может быть полезен отдельный чатик для призыва ревьюверов в PR) email уведомления тоже полезны, но почту читают реже, письма могут попасть в спам
#code_review
GitHub
GitHub - spotbugs/sonar-findbugs: SpotBugs plugin for SonarQube
SpotBugs plugin for SonarQube. Contribute to spotbugs/sonar-findbugs development by creating an account on GitHub.
Всем привет!
До сих пор говоря про ревью кода, я акцентировал внимание на команде. Когда же может быть полезно кросс-командное ревью.
Основной кейс, когда внешнее ревью обязательно - над общей кодовой базой ведет работу несколько команд. Внешний ревьювер здесь играет роль независимого аудитора. Ведь правки одной команды могут что-то сломать для другой команды. Или новая команда может нарушить единообразие архитектуры.
Привлечение ревьюверов из другой команды может быть полезно, если не хватает опыта по используемой платформе или технологии.
Также кросс-ревью может быть полезно, если модули, выпускаемые командами, интегрированы друг с другом. С одной стороны может показаться, что это нарушает инкапсуляцию, которую дает нам API. Но бывают случае, когда между "соседними" командами возникает недопонимание - почему API сделано именно так или почему его стали использовать именно таким образом. Для начала в таких случаях стоит собраться и обсудить API. Если же этого мало - возможно, погружение в код соседей поможет понять причины.
Еще один вариант призыва внешнего ревьювера(ов), назовем его "пожарная команда" - если в команде в данный момент не хватает ревьюверов по причине отпуска, увольнения или набора новой команды.
И последний кейс - внешний ревьювер как арбитр для разрешения споров внутри команды. Как правило это техлид или просто опытный разработчик.
#code_review
До сих пор говоря про ревью кода, я акцентировал внимание на команде. Когда же может быть полезно кросс-командное ревью.
Основной кейс, когда внешнее ревью обязательно - над общей кодовой базой ведет работу несколько команд. Внешний ревьювер здесь играет роль независимого аудитора. Ведь правки одной команды могут что-то сломать для другой команды. Или новая команда может нарушить единообразие архитектуры.
Привлечение ревьюверов из другой команды может быть полезно, если не хватает опыта по используемой платформе или технологии.
Также кросс-ревью может быть полезно, если модули, выпускаемые командами, интегрированы друг с другом. С одной стороны может показаться, что это нарушает инкапсуляцию, которую дает нам API. Но бывают случае, когда между "соседними" командами возникает недопонимание - почему API сделано именно так или почему его стали использовать именно таким образом. Для начала в таких случаях стоит собраться и обсудить API. Если же этого мало - возможно, погружение в код соседей поможет понять причины.
Еще один вариант призыва внешнего ревьювера(ов), назовем его "пожарная команда" - если в команде в данный момент не хватает ревьюверов по причине отпуска, увольнения или набора новой команды.
И последний кейс - внешний ревьювер как арбитр для разрешения споров внутри команды. Как правило это техлид или просто опытный разработчик.
#code_review