test(snapshot): add mock-based tests after snapshot interface refactor#130
test(snapshot): add mock-based tests after snapshot interface refactor#130
Conversation
| } | ||
|
|
||
| disks, err := listFileDisks(domain) | ||
| xmlDesc, err := domain.XMLDesc() |
There was a problem hiding this comment.
domain.XMLDesc와 listFileDiskFrom* 함수는 나중에 pkg/virtxml 등으로 이전하거나 해당 함수를 가져다 쓰는방식이 좋을 것 같아요
나중에 virtxml 구현완료되면 한번에 이전하겠습니다
| snapshot *libvirt.DomainSnapshot | ||
| } | ||
|
|
||
| func (s *libvirtInternalSnapshotHandle) Name() (string, error) { |
There was a problem hiding this comment.
이 4개 함수는 단순 구조체 삽입만으로도 인터페이스 조건을 충족할 수 있을 거 같아요.
- 호출 부분을 internal 과 같은 곳으로 옮기는 것도 괜찮아보이네요
/internal/
type libvirtInternalSnapshot interface{
GetName() //libvirt.snapthot 의 시그니쳐와 동일하게
Delete()
Revert()
....
}
| "libvirt.org/go/libvirt" | ||
| ) | ||
|
|
||
| type externalSnapshotDomain interface { |
There was a problem hiding this comment.
인터페이스명을 이렇게 길게 쓰지 않아도 될 거 같아요.
내부 구조체라면, external 관련 메소드가 있는 것을 알기 때문에 SnapshotDomain으로도 충분해 보이고,
외부 구조체라도 호출시 external.SnapshotDomain 이면 충분히 이해가능해보입니다.
또한 메소드들을 보면 externalSnapshotDomain 의 성격과 다른 메소드들이 많이 보입니다.
| AbortBlockJobPivot(disk string) error | ||
| UpdateDeviceConfig(deviceXML string) error | ||
| UUIDString() (string, error) | ||
| XMLDesc() (string, error) |
There was a problem hiding this comment.
몇몇 함수들은 externalSnapthotDomain 이라는 인터페이스의 역할과 묶여있지 않아도 될 것 같습니다.
대다수의 함수는 단순 구조체 삽입만으로도 충분해보여요.
단순 libvirt.Domain 을 임베딩하면서 충족가능한 메소드와 그걸 활용하는 메소드들이 같은 인터페이스에 있는 것을
경계해주시면 될 것 같습니다.
이 둘을 분리시켜서, libvirt.Domain 으로 충족가능한 메소드들을 아래 레이어로 분리시키고, 그걸 활용하는 함수들을 여기에 두는게 맞을 것 같아요.
libvirt.domain 삽입으로 해결 가능한가? 그럼 최하 레이어로
libvirt.Domain 삽입으로 해결 가능하지만 libvirt 종속적인가(패키지를 호출해야되는가) pkg 혹은 internal로
다른 서비스들에서 공통적으로 쓰는 로직인가 internal로 옮기기.
현재 다른 서비스들은 로직자체가 단순하기 때문에 domain 인터페이스가 서비스단에 있지만,
snapshot 은 로직자체가 복잡해서 분리시켜놓는게 적절해보입니다.
| domain *libvirt.Domain | ||
| } | ||
|
|
||
| func newExternalSnapshotDomain(domain *libvirt.Domain) externalSnapshotDomain { |
There was a problem hiding this comment.
이렇게 Domain삽입해서 인터페이스 생성하는 것은 아주 좋은것 같습니다.
| return d.domain.IsActive() | ||
| } | ||
|
|
||
| func (d *libvirtExternalSnapshotDomain) CreateExternalSnapshot(snapshotXML string, opts externalSnapshotCreateExecOptions) (externalSnapshotHandle, error) { |
There was a problem hiding this comment.
위의 IsActive와 CreateExternalSnapshot이 좋은 예시가 될 것 같은데,
IsActive 는 단순 임베딩으로 충족가능할 뿐만아니라 로직자체도 단순하지만,
CreateExternalSnapshot 은 libvirt 를 호출할뿐만아니라 로직도 복잡합니다.
위 두 메소드가 같은 인터페이스에 속해있으면 혼란스러울 수 있을 것 같아요.
만약 저라면, IsActive 로직은 단순 삽입으로,
CreateExternalsnapshot 은 libvirt 플래그 종속적이기때문에 internal로 둘 것 같습니다
|
파일 분리, XMLvirt는 해당 pr 에서 신경쓰지 않아도 될 것 같습니다. |
2 participants
Type of Change
Summary
+) 기존에 진행하기로 했던 qemu-img 기반 기능 개선이 이뤄지면 세부내용이 변경될 수도 있습니다.
Related Issue
Closes #
Test Plan
make testpasses