Merged
Conversation
platanus-kr
requested review from
S0okJu,
choieastsea,
halucinor,
jja6312 and
paikend
platanus-kr
added
the
feature
platanus-kr
changed the title
S0okJu
requested changes
Oct 14, 2025
Collaborator
S0okJu
left a comment
S0okJu
left a comment
There was a problem hiding this comment.
현재 코드에는 security group의 속성만 관리하고, security group rule를 관리(추가 및 해제) 기능이 생략된 것 같습니다.
이 부분에 대해 추가 계획이 있으신가요?
| if isinstance(r, dict): | ||
| rid = r.get("id") | ||
| else: | ||
| rid = getattr(r, "id", None) |
Collaborator
There was a problem hiding this comment.
getattr 으로 속성을 확인하신 이유가 궁금합니다.
Collaborator
Author
There was a problem hiding this comment.
security group에 rule이 없을때 None을 반환하는 로직입니다~
아래 승주님 의견받아 rule 객체로 변환하면서 해당 로직은 사라졌습니다!
halucinor
requested changes
Oct 14, 2025
Comment on lines
1273
to
1289
| def _convert_to_security_group_model(self, openstack_sg) -> SecurityGroup: | ||
| """ | ||
| Convert an OpenStack Security Group object to a SecurityGroup pydantic model. | ||
|
|
||
| :param openstack_sg: OpenStack security group object | ||
| :return: Pydantic SecurityGroup model | ||
| """ | ||
| rule_ids: list[str] | None = None | ||
| rules = getattr(openstack_sg, "security_group_rules", None) | ||
| if rules is not None: | ||
| extracted: list[str] = [] | ||
| for r in rules: | ||
| rid = None | ||
| if isinstance(r, dict): | ||
| rid = r.get("id") | ||
| else: | ||
| rid = getattr(r, "id", None) |
Collaborator
There was a problem hiding this comment.
private 함수 없이 Pydantic 객체로 변환하는게 불가능한가요?, 복잡한 변환이 아니여서 함수없이도 충분히 가능해보입니다.
Collaborator
Author
There was a problem hiding this comment.
문자열 리스트가 아닌 보안그룹 규칙 객체로 반환하도록 개선하겠습니다!
Collaborator
Author
halucinor
reviewed
Oct 14, 2025
jja6312
approved these changes
Oct 16, 2025
Collaborator
jja6312
left a comment
jja6312
left a comment
There was a problem hiding this comment.
저는 한 가지 의견을 제외하고는 이견이 없습니다!👍
| def get_security_groups( | ||
| self, | ||
| project_id: str | None = None, | ||
| name: str | None = None, |
Collaborator
There was a problem hiding this comment.
이미 훌륭한 코드에 한 가지 제 생각을 덧붙여보자면, id필드가 있으면 더 좋지 않을까 생각이 듭니다.
id가 필요없을까? 생각에서 뻗어나갔는데요,
security group은 name을 중복값으로 생성할 수 있을텐데,
만약 조금 복잡하게 질문해서
- 전제: 중복된 security name이 많음
- 프롬프트: "instance중에 tag가 prod인 인스턴스들을 조회하고, 그 인스턴스들이 어떤 security group을 쓰는지 조회해줘"
라고 했을 때,
기존 프로젝트의 compute에서는 return은 id와 name 모두를 반환하므로 get_security_group_detail 함수에 id를 통해 정확히 조회해준다면 아름답겠지만, 자칫 get_security_groups의 name으로 조회하게된다면, 결과를 보장하기 힘들 것 같습니다.
그런데 id필드가 있으면 적어도 이 케이스에 대해서는 안전할 것 같습니다
Collaborator
Author
There was a problem hiding this comment.
그러네요 ID 필드 추가하도록 하겠습니다!
halucinor
reviewed
Oct 16, 2025
halucinor
reviewed
Oct 18, 2025
halucinor
requested changes
Oct 18, 2025
halucinor
pushed a commit
that referenced
this pull request
Oct 23, 2025
* feat(network): Add security group tools (#85) * improve(network): change to convert security group rules into objects during security group conversion (#85) * improve(network): remove unnecessary None checks for network, subnet, port, floating ip, router, security group (#85) * fix(network): add id field to get security group (#85) * fix(network): remove unnecessary None checks (#85) * fix(network): integrate test (#85)
halucinor
pushed a commit
that referenced
this pull request
Oct 24, 2025
* feat(network): Add security group tools (#85) * improve(network): change to convert security group rules into objects during security group conversion (#85) * improve(network): remove unnecessary None checks for network, subnet, port, floating ip, router, security group (#85) * fix(network): add id field to get security group (#85) * fix(network): remove unnecessary None checks (#85) * fix(network): integrate test (#85)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Key Changes
Related Issues
Additional context