Skip to content

initial implementation of LicenseAPI#7

Merged
shyouhei merged 3 commits into
feature/shyouhei/ogenfrom
feature/shyouhei/licenses
Oct 3, 2025
Merged

initial implementation of LicenseAPI#7
shyouhei merged 3 commits into
feature/shyouhei/ogenfrom
feature/shyouhei/licenses

Conversation

@shyouhei

Copy link
Copy Markdown
Member

/cloudhsm/licenses に関連する部分です。

Signed-off-by: 卜部 昌平 <s-urabe@sakura.ad.jp>
@shyouhei shyouhei requested a review from repeatedly September 30, 2025 02:18

@repeatedly repeatedly left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

気になったところをコメントしておきました。

Comment thread license.go Outdated
}

type CloudHSMSoftwareLicenseCreateParams struct {
ServiceClass v1.CloudHSMSoftwareLicenseServiceClassEnum

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ServiceClassをユーザが指定したいことはないと思うので、Paramsから削除してしまった方が混乱が少ないかと思います。Updateの方も同様です。

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

  • Create: 現状一種類しかないのでべつに指定してもしなくても同じ、という意味では、まあ不要かなとも思います。将来的にenumが増えた時にどうするか? は一考の余地ありか
  • Update: 実はこのAPIにはPATCHがなくて全部PUTなのですね、なので差分更新みたいなことはできず、全部指定し直す必要がある。今はどうせ固定値だからどうでもいいとして、将来、可変になることを見越すのであれば、サービスクラスも指定できるようにしておかないと壊れてしまうなという感じです。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ServiceClassはそもそもcloud-ctrlがサービスを分岐するためのものでユーザが指定するためのものではないと思うので、ユーザ側から指定することはないかなと思います(他のサービスだとそもそもこれと同等のフィールドがreadOnlyだったり)。
もしこれをユーザが指定する必要に迫られるのであれば、多分API自体が変更になっている可能性が高いと思われます。

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

0125799 消してみました

Comment thread license.go Outdated
Comment thread license.go Outdated
} else if e.StatusCode == http.StatusUnprocessableEntity {
return nil, NewAPIError("License.Update", e.StatusCode, errors.Wrap(err, "invalid parameter"))
} else {
return nil, NewAPIError("License.Update", 0, err)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

他のアクションではここがinternal server errorになってますが、ここだけ違うのは意図的ですか?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

あ、これはミスです。ありがとうございます。

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

8535387 で対応済みです

Signed-off-by: 卜部 昌平 <s-urabe@sakura.ad.jp>
There is no alternative to the field.

Signed-off-by: 卜部 昌平 <s-urabe@sakura.ad.jp>

@repeatedly repeatedly left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@shyouhei shyouhei merged commit ebd9c4f into feature/shyouhei/ogen Oct 3, 2025
8 checks passed
@github-actions github-actions Bot mentioned this pull request Nov 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants