feat(CLOUDDEV-420): added disable_port_security option to instance re… #38
Conversation
DamirZinatullin
force-pushed
the
feat/CLOUDDEV-420-port-security
branch
4 times, most recently
from
49ede8d to
73e8b5d
Compare
docs/resources/cdn_sslcert.md
Outdated
| @@ -48,5 +48,3 @@ resource "edgecenter_cdn_sslcert" "cdnopt_cert" { | |||
| - `automated` (Boolean) The way SSL certificate was issued. | |||
| - `has_related_resources` (Boolean) It shows if the SSL certificate is used by a CDN resource. | |||
| - `id` (String) The ID of this resource. | |||
|
|
|||
There was a problem hiding this comment.
Старайся делать изменения в форматировании отдельно от бизнес-логики, потому что иначе не удобно смотреть MR: много лишних файлов в списке изменений и приходится фильтровать нужные
Если все-таки необходимо сделать в одном PRе, выноси форматирование в отдельный коммит, когда это возможно
| @@ -382,6 +417,43 @@ func attachInterfaceToInstance(instanceClient *edgecloud.ServiceClient, instance | |||
| return err | |||
| } | |||
|
|
|||
| interfacesListAPI, err := instances.ListInterfacesAll(instanceClient, instanceID) | |||
There was a problem hiding this comment.
Когда функция большая и состоит из нескольких этапов, можно выделить явные шаги. Например, выше у тебя происходил attach интерфейса, а ниже приведение поля disable_port_security в соответствии с его значением в opts.
Я обычно подписываю такие шаги, чтобы легче было читать код.
Предлагаю тебе еще раз взглянуть на твой код и посмотреть, будет ли здесь актуально тоже подписать, что это за большой шаг внутри функции
There was a problem hiding this comment.
Долго читал функцию, особенно меня смутило, почему у тебя в цикле проверяется opts, если opts -- это объект, сформированный только для запроса выше в функции.
Это наглядный пример, почему стоит когда это возможно следить за тем, чтобы функция выполняла ровно одну задачу. Иначе нужно внимательно следить, чтобы переменные, предназначеные для чего-то одного, не использовались там, где они не предназначена.
| ipAddress := assignment.IPAddress.String() | ||
|
|
||
| var portSecurityDisabled bool | ||
| for _, interfaceExtracted := range portSecurityOptsListExtracted { |
There was a problem hiding this comment.
Здесь явно путаница в типах, отмечу те проблемы, которые заметил:
- Из того, что я заметил, всё что
optsобозначает параметры для запросов к Cloud API. У тебяInstancePortSecurityOptsкажется не используется в таком ключе, обозначает что-то другое. - В этой функции непонятно чем отличается
portSecurityOptsListотportSecurityOptsListExtracted: они имеют одинаковый тип, и первый собирается из второго. Это мешает разобраться, что делает код. - И третье - в строке
for _, interfaceExtracted := range portSecurityOptsListExtracted {у тебя то, что былоopts, превращается вinterface, что окончательно вносит путаницу
Проверь, пожалуйста, и поправь, если действительно есть проблема здесь
DamirZinatullin
force-pushed
the
feat/CLOUDDEV-420-port-security
branch
from
3668fda to
4c08398
Compare
|
Стало проще и легче читать, аппрувнул |
DamirZinatullin
force-pushed
the
feat/CLOUDDEV-420-port-security
branch
from
158885f to
cee1e84
Compare
DamirZinatullin
force-pushed
the
feat/CLOUDDEV-420-port-security
branch
from
cee1e84 to
048524f
Compare
…source