-
Notifications
You must be signed in to change notification settings - Fork 6
feat: Add get attachments tool #84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
0f071d4
feat(block): Add get_attachment_id tool(#70)
S0okJu 0b84ea2
fix(block): Delete tool counting test
S0okJu e649122
feat(cinder): Add get attachments tool(#70)
S0okJu 21121d2
Merge branch 'develop' into feat/volume-attachment
S0okJu 103b84c
feat(cinder): Add get attachments tool(#70)
S0okJu ff266b0
feat(cinder): Add get attachments tool(#70)
S0okJu a2394d3
fix(block): Change response object to Attachment in a get_attchments …
S0okJu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 보기엔 거의 훌륭하게 작성해주셨는데요,
@platanus-kr 님 comment를 보고 생각해본 바로는
docstring에 instance에 대해 instance id 대신 공식문서의 표현을 차용하는것은어떨까요?("volume attachments like nova, glance, ironic etc." 라는 구절이 있으니.. 이러한 것들의 id다 정도로요.)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일부 백엔드의 경우에는 instance id가 필요해요.
말씀해주신대로 다양한 백엔드를 지원하려면 백엔드 종류에 해당되는 인자를 추가하는 것이 좋을 것 같아요.
현재로써는 다양한 환경에서 테스트를 못하다보니 해당 기능은 보류하게 되었어요.
관련해서 이슈를 작성하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음..인자를 추가하신다는게 제가 잘 이해가 안가는데요!
instance 인자가 nova뿐만아니라 다양한 block volume의 소비 주체를 포괄하고있다고 저는 이해했는데,
그 말인 즉슨 다양한 백엔드를 커버하기 위해서는 다른 인자를 추가하지 않아도 현재 코드가 이미 완성형이고, 나중에 추가되는 툴마다 테스트만 추가하면 되지않나 하는 의견이었습니다!
@S0okJu 님 의견은 get_attachments가 지금 구현하지 않은 백엔드툴들을 포괄하는 기능 구현이므로 테스트를 충분히 할 수 없으니 보류하겠다는 말씀이신거죠?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jja6312 말씀이 맞습니다.
해당 응답값은 완성형이며, 응답값만으로도 다양한 백엔드를 지원할 수 있습니다.
말씀해주신 것처럼
instacne_id를 다른 변수 이름으로 바꾼다면 사용자 측면에서 좋을 것 같습니다.다만, 백엔드에 따라 어떤 형태로 반환되는지 직접 확인하고 변경하고 싶습니다.
네, 직접 확인한 후에 이 사안에 대해서 반영하겠습니다.