feat: migrate aws-sdk-go from v1 to v2 and remove vendor directory#181
Conversation
| fmt.Println(err.Error()) | ||
| var notFound *dbtypes.ResourceNotFoundException | ||
| if errors.As(err, ¬Found) { | ||
| return false, nil |
There was a problem hiding this comment.
Println으로 에러 출력하지 않는 걸로 보이는데 맞나요?
There was a problem hiding this comment.
이 부분은 error가 없어서 return fasle, nill로 처리하고 아래에서 출력하도록 변경
| // Message from an error. | ||
| fmt.Println(err.Error()) | ||
| } | ||
| fmt.Println(err.Error()) |
There was a problem hiding this comment.
기존은 managed exception이랑 그외로 나눠서 error 출력하는 걸로 보이는데,
Error 존재 시 일괄 출력하는 방식으로 바꾸신 이유가 있나요?
There was a problem hiding this comment.
위와 동일하게 fmt로 출력하도록 변경
| // Message from an error. | ||
| fmt.Println(err.Error()) | ||
| } | ||
| fmt.Println(err.Error()) |
There was a problem hiding this comment.
| VolumeSize: int64Ptr(30), | ||
| VolumeType: stringPtr("gp2"), | ||
| Ebs: &ec2types.LaunchTemplateEbsBlockDeviceRequest{ | ||
| VolumeSize: int32Ptr(30), |
There was a problem hiding this comment.
int64Ptr에서 int32Ptr로 변경하는 이유가 있나요?
e.g., API 스펙, 모델이 int32를 요구함
There was a problem hiding this comment.
| { | ||
| name: "Valid security groups without ENI", | ||
| securityGroups: []*string{aws.String("sg-12345678")}, | ||
| securityGroups: []string{"sg-12345678"}, |
There was a problem hiding this comment.
helper functions 호출 안하는 이유가 있나요?
There was a problem hiding this comment.
aws.String()이나 stringPtr()는 *string (포인터)이 필요할 때 쓰는 헬퍼고, []string은 그냥 값 타입 슬라이스라서 헬퍼가 필요 없음.
There was a problem hiding this comment.
포인터를 안 쓰게 되면서 string을 사용하게 됨. string을 사용하게 되니 helper함수 쓸 필요가 없음으로 이해하겠습니다.
|
|
||
| "github.com/aws/aws-sdk-go-v2/aws" | ||
| astypes "github.com/aws/aws-sdk-go-v2/service/autoscaling/types" | ||
| elb "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancing" |
There was a problem hiding this comment.
elb.go는 CLB용, elbv2.go는 최신 API용으로 이해했습니다.
v2로 업그레이드하면서 elb.go는 제거하고 elbv2.go만 사용하도록 바꾸는 건 어떠세요?
사용 사례들 호환성 때문에 불가능하다면 그냥 두는 게 낫고요.
service/elasticloadbalancing
Classic Load Balancer(CLB)용 API 클라이언트입니다. AWS 문서/레퍼런스에서 이 세대는 API 2012-06-01로 구분됩니다.
service/elasticloadbalancingv2
“현재 세대” 로드 밸런서인 ALB(Application), NLB(Network), GLB(Gateway) 용 API 클라이언트입니다. 이 세대는 API 2015-12-01로 구분됩니다.
There was a problem hiding this comment.
이후에 해당 부분 v2로 변경
| func NewELBClient(cfg aws.Config) ELBClient { | ||
| return ELBClient{ | ||
| Client: getELBClientFn(session, region, creds), | ||
| } | ||
| } | ||
|
|
||
| func getELBClientFn(session client.ConfigProvider, region string, creds *credentials.Credentials) *elb.ELB { | ||
| if creds == nil { | ||
| return elb.New(session, &aws.Config{Region: aws.String(region)}) | ||
| Client: elb.NewFromConfig(cfg), |
There was a problem hiding this comment.
getter method가 아닌 직접 참조 방식으로 바꾸신 이유가 있을까요?
e.g., 나중을 고려해봐도 별도 처리 로직 필요 없을 것으로 확신함
There was a problem hiding this comment.
이전에는 session을 별도로 불러와서 config를 가져오는 방식이었는데 v2에서는 aws.Config를 load하고 이를 직접 참조하는 방식으로 바뀜
v1: https://github.com/aws/aws-sdk-go/blob/070853e88d22854d2355c2543d0958a5f76ad407/aws/client/client.go#L54
v2: https://github.com/aws/aws-sdk-go-v2/blob/main/aws/retry/internal/mock/client.go
There was a problem hiding this comment.
v2 링크가 mock 패키지 하위의 client.go링크로 보입니다!
There was a problem hiding this comment.
https://github.com/aws/aws-sdk-go-v2/blob/790446eef174929d0796cbd608c43cdea0bba443/service/elasticloadbalancing/api_client.go#L440
이 부분으로 이해하겠습니다 :)
New* 함수 자체에서 유효성 검사 후 return하는 것으로 이해.
| } | ||
|
|
||
| result, err := e.Client.DescribeInstanceHealth(input) | ||
| result, err := e.Client.DescribeInstanceHealth(context.Background(), input) |
There was a problem hiding this comment.
input 객체만 하나만 argument로 전달하게끔
https://github.com/DevopsArtFactory/goployer/pull/181/changes#diff-8b3e0da4d99143e7fa0a2b2e0dc3c2b87d15da8a0fa87634e683e1f10ae58538R42-R44 여기에 context.Background()를 설정할 수 있나요?
There was a problem hiding this comment.
| } | ||
|
|
||
| tgWithDetails, err := e.DescribeTargetGroups(aws.StringSlice(targetGroups)) | ||
| tgWithDetails, err := e.DescribeTargetGroups(targetGroups) |
There was a problem hiding this comment.
StringSlice로 값 유효성 체크 또는 포매팅할 겸 그대로 유지하는 건 어떠세요? 나머지 7건 모두 포함
| for _, hd := range result.TargetHealthDescriptions { | ||
| if *hd.Target.Id == *instance.InstanceId { | ||
| targetState = *hd.TargetHealth.State | ||
| targetState = string(hd.TargetHealth.State) |
There was a problem hiding this comment.
이런 부분들 aws.StringSlice() 메서드로 처리가능한 지 보면 좋을 것 같아요.
| CurrentTime time.Time | ||
| TargetGroups []*string | ||
| LoadBalancers []*string | ||
| TargetGroups []string |
There was a problem hiding this comment.
[]*string -> []string으로 변경하는 이유가 있을까요?
API spec이 그렇게 변경되었거나, nil 체크할 필요가 없음
There was a problem hiding this comment.
위에서 언급한거처럼 v2로 가면서 포인터를 넘길 필요가 없어짐
|
|
||
| // inbound | ||
| if err := client.EC2Service.UpdateInboundRulesWithGroup(*groupID, "tcp", "Allow access from canary load balancer", lbSg, *tg.Port, *tg.Port); err != nil { | ||
| if err := client.EC2Service.UpdateInboundRulesWithGroup(*groupID, "tcp", "Allow access from canary load balancer", lbSg, int64(*tg.Port), int64(*tg.Port)); err != nil { |
There was a problem hiding this comment.
int64, int32 각각 처리할 때의 기준이 있을까요?
추가로 아래 케이스에서는 64 -> 32로 변경했었는데요 64가 아닌 32가 적절할 수 있을것 같은데(Port 0 ~ 65535) 어떻게 생각하시나요?
https://github.com/DevopsArtFactory/goployer/pull/181/changes#r2875983501
There was a problem hiding this comment.
UpdateInboundRulesWithGroup method를 보시면 port를 둘다 int64로 받고 있습니다.
말한 변경사항은 sdk의 변경사항입니다.
| groupID, err := client.EC2Service.CreateSecurityGroup(lbSGName, tg.VpcId) | ||
| if err != nil { | ||
| if aerr, ok := err.(awserr.Error); ok && aerr.Code() == "InvalidGroup.Duplicate" { | ||
| if strings.Contains(err.Error(), "InvalidGroup.Duplicate") { |
There was a problem hiding this comment.
v2로 호출 API 변경되면서 error 처리 로직 변경으로 이해했습니다.
Chaaany
left a comment
There was a problem hiding this comment.
전반적으로 aws V2 API call의 spec 변경으로 인한 유지보수 작업으로 이해하였습니다.
API spec 변경으로 인한 Error/Exception handling, parameter type 변경(pointer가 아닌 객체 type으로 변경)은 이의 없으나,
일부 Getter method 제거 및 수정, method argument/parameter 추가/제거 부분에 있어서 궁금한 점에 대해 comment 남겼습니다.
Chaaany
left a comment
There was a problem hiding this comment.
goployer 동작에는 문제 없는 것으로 보여, 코멘트와 함께 승인합니다.
코멘트 중 수정사항 있으면 추가 PR로 작업 부탁드립니다.

Description
Migrate from
aws-sdk-gov1 toaws-sdk-go-v2for improved performance, modularity, and long-term support.As part of this migration, the vendor directory has been removed from Git tracking and added to
.gitignore, since Go Modules (go.mod) handles dependency management and vendoring is no longer necessary.vendor/to.gitignoreUser facing changes (remove if N/A)
N/A
Follow-up Work (remove if N/A)
N/A