[ECS] Refactored from json.RawMessage to ECSTargetGroup struct

[t-kikuc]

What this PR does / why we need it:

Internal refactoring.

For pipectl init command and ECS redesigning, it's more convenient to have struct than json.RawMessage.

Which issue(s) this PR fixes:

no

Does this PR introduce a user-facing change?: no

• How are users affected by this change: no
• Is this breaking change: no
• How to migrate (if breaking change): no

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2cbcef3) 30.80% compared to head (28dfdce) 30.85%.
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4737      +/-   ##
==========================================
+ Coverage   30.80%   30.85%   +0.04%     
==========================================
  Files         221      222       +1     
  Lines       26015    26037      +22     
==========================================
+ Hits         8015     8033      +18     
- Misses      17349    17355       +6     
+ Partials      651      649       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[t-kikuc]

/review

[github-actions]

PR Analysis

Main theme

Refactoring ECSTargetGroups structure and loading mechanism.

PR summary

This pull request refactors the code for loading ECS target groups by replacing the previous JSON decoding mechanism with direct structure initialization. The PR modifies the ECSTargetGroups struct to no longer use raw JSON messages but instead use a nested ECSTargetGroup struct.

Type of PR

Refactoring

PR Feedback:

General suggestions

The refactor from using json.RawMessage and decoding the JSON to using a nested struct ECSTargetGroup simplifies the code and makes it more type-safe by avoiding runtime JSON unmarshalling errors. This change should make the code easier to understand and less error-prone when it comes to future maintenance or additional features. However, a thorough testing process is advised to ensure that the transition from dynamic JSON structures to statically typed structures does not introduce potential issues with deserialization of previously persisted configurations or API changes that might affect users who update the application.

Code feedback

• relevant file: pkg/app/piped/platformprovider/ecs/target_groups.go
  suggestion: Ensure that the targetGroups.Primary and targetGroups.Canary are not nil before initializing the types.LoadBalancer instances to avoid potential nil pointer dereference. This is particularly important for optional configurations where canary might not be declared.
  relevant line: + TargetGroupArn: aws.String(targetGroups.Primary.TargetGroupArn),

• relevant file: pkg/config/application_ecs.go
  suggestion: Consider including validation logic for the new ECSTargetGroup to ensure that required fields are provided and that ContainerPort is within a valid range for ECS services. This could help catch configuration issues early, rather than during runtime.
  relevant line: + ContainerPort int json:"containerPort"``

Security concerns:

no

The PR does not introduce changes in logic that affect security; it's purely refactoring existing configurations. The newly introduced types.LoadBalancer objects are dependent on values provided by the user, which were already being used in the previous implementation, thus not introducing new security issues.

[khanhtc1202]

I don't have a strong opinion about this change. The current implementation uses json.RawMessage is due to the fact that we would keep it changeable. But since we're redesigning the whole ECS configuration, maybe changing this doesn't make too much sense 🤔 wdyt? @t-kikuc cc @ffjlabo

[ffjlabo]

@khanhtc1202
IMO, I agree to refactor it.
It looks like it is for the implementation of pipectl init. Maybe a concrete type is needed when creating config file, right? @t-kikuc

[t-kikuc]

@khanhtc1202

It looks like it is for the implementation of pipectl init. Maybe a concrete type is needed when creating config file, right?

Yes! Thanks @ffjlabo

We can decide the ECSTargetGroupObject's fields as the doc below, and we can modify if needed in the future.
https://pipecd.dev/docs-v0.45.x/user-guide/configuration-reference/#ecstargetgroupobject

Besides, I would not modify the ECSTargetGroupObject itself soon for redesigning ECS.

[khanhtc1202]

👍

[ffjlabo]

🚀