Skip to content
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

Fix some REST APIs methods from get to post #742

Merged
merged 2 commits into from
Sep 15, 2021

Conversation

atg0831
Copy link
Member

@atg0831 atg0831 commented Sep 15, 2021

fix #674
lookupspec, lookupimage 관련 HTTP method를 get에서 post로 변경 및 update swagger docs

  • cb-tumblebug/src/api/rest/server/server.goe.GET -> e.POST로 변경
  • Go swagger annotation [get] -> [post]로 변경
  • CB-Tumblebug test scripts for REST APIs의 script 파일의 method GET -> POST로 변경
  • cb-tumblebug/src 에서 make swag 으로 swagger docs 업데이트
  • CB-TB 기여자의 테스팅 준수 사항을 지키면서 테스트도 정상적으로 완료했습니다.

swagger docs 업데이트 하는 과정에서 "github.com/alecthomas/template" package 대신 "text/template" package가
자동으로 import 되었는데 이 부분에 대해서 조언을 구하고 싶습니다.

Fix REST API'S Methods related to 'lookupspec` and `lookupimage' from 'get' to 'post', because methods need json body

Below is fixed part
- route for method(e.GET -> e.POST)
- go swagger annotation for Swagger doc([get] -> [post])
- test scripts for REST APIs(GET -> POST)
parts of method change(get -> post)
- lookupImage, lookupImages
- lookupSpec, lookupSpecs
package docs

import (
"bytes"
"encoding/json"
"strings"
"text/template"
Copy link
Member Author

@atg0831 atg0831 Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

swagger docs를 업데이트 하는 과정에서 이 package가 자동으로 import 되고 기존의 "github.com/alecthomas/template" package가 빠졌습니다.
동시에 아래 코드에서 "text/template" package가 import 되면서 escape문이 추가 되었는데 이렇게 진행을 해도 괜찮을까요?

"description": "{{escape .Description}}",

"escape": func(v interface{}) string {
// escape tabs
str := strings.Replace(v.(string), "\t", "\t", -1)
// replace " with ", and if that results in \", replace that with \"
str = strings.Replace(str, """, "\"", -1)
return strings.Replace(str, "\\"", "\\\"", -1)
},

Copy link
Member

@jihoon-seo jihoon-seo Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atg0831 기여 감사합니다 😊

혹시 사용하고 계신 swag 버전을 알 수 있을까요? 😊
저는 확인해보니 v1.7.0 입니다.

❯ swag -v
swag version v1.7.0

현재 최신버전은 v1.7.1 이네요 😊
https://github.com/swaggo/swag/releases

v1.7.1 changelog 중에
"chore: alecthomas/template -> text/template" 가 있는 것으로 봐서
v1.7.1 을 사용중이신 것 같군요.. ㅎㅎ

저도 v1.7.1 로 업데이트하고 make swag 실행하니
이 PR과 동일해졌습니다.

이 PR대로 진행하면 될 것 같습니다. 😊

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jihoon-seo
혹시,
wiki/API-Document-UpdateReadme Swagger API 문서 업데이트 필요 시 항목에 swag version 에 대한 명시가 필요할까요?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seokho-son 훔..
기여자가 앞으로 v1.7.1 이 아닌 v1.7.0 등을 사용하면
code pingpong이 계속 일어날 수 있기는 합니다.

명시를 하는 것도 좋을 것 같은데,
그러면 앞으로 swag 이 업데이트 될 때마다 저희도 (필요 시) 업데이트를 해야 하겠네요 😊

비슷한 이슈로 protoc 버전이 있습니다.

과거에 있었던 swag / protoc code pingpong 이슈를 생각하니, 버전 명시를 하는 것이 좋을 것 같다는 생각이 더 드네요 😊

Copy link
Member

@yunkon-kim yunkon-kim Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seokho-son @jihoon-seo
위에서 말씀하신 protoc 관련하여 어제 고생을 조금 한터라 😭 간단히 정리한 내용을 공유 드립니다 ☺️

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jihoon-seo @seokho-son
네 제가 swag 최신 버전을 사용했고 Makefile을 이용해서 make swag을 했는데요,
그런데 지금 보니가 cb-tumblbug의 go.mod 파일에는 swag v1.7.0 으로 되어 있네요.
go.mod의 swag의 버전과 $GOPATH의 bin 디렉토리 안의 swag의 버전이 다른데 일치시켜야 하지 않을까요?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atg0831go.modswag 버전도 업데이트 해야겠네요 😊

  • 이 PR에서 업데이트를 해 주셔도 좋고,
  • 아니면 이 PR이 머지된 이후에 별도의 PR로 업데이트를 해도 좋을 것 같습니다. 😊

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jihoon-seo
이번 PR에서 말고 별도의 PR로 수정해서 올리도록 하겠습니다!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atg0831 이슈로 미리 올리고 작업 시작하시면 좋을 것 같습니다! ^^
이슈 올리실 때,
image
를 활용하는 방법도 있어요 ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seokho-son
팁 알려주셔서 감사합니다 ㅎㅎ 이슈 올려서 진행해볼게요

@jihoon-seo jihoon-seo added the lgtm This PR is acceptable by at least one reviewer label Sep 15, 2021
@seokho-son
Copy link
Member

@atg0831 기여 감사합니다..! LGTM too!

@seokho-son
Copy link
Member

/approve

@seokho-son seokho-son merged commit e154342 into cloud-barista:main Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR is acceptable by at least one reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix some REST API's HTTP methods
4 participants