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

🎨 remove NewOAuthAPI function, remove HTTPClient field's assignment #565

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

iChemy
Copy link
Contributor

@iChemy iChemy commented Aug 26, 2024

HTTPClient フィールドを書き換える処理が原因で不具合が起きていることが確認できたので
HTTPClient フィールドを書き換えないような処理に変更しました.

@iChemy
Copy link
Contributor Author

iChemy commented Aug 26, 2024

グローバル変数じゃなくして
#515
のときのような実装に戻すという選択肢もありますが

  • そもそも go-traq のドキュメントには HTTPClient フィールドを書き換える様なことをしている部分は見られない,
  • サーバーのアクセストークンを使う場合は WithValue を使用している

という点から全て WithValue に統一した方が良いと考えました.

@iChemy iChemy requested a review from Nzt3-gh August 26, 2024 11:31
@iChemy
Copy link
Contributor Author

iChemy commented Aug 26, 2024

@Nzt3-gh

traq.ConfigurationHTTPClient フィールドを書き換えるように実装した理由ってあったりしますか?

ctx := context.TODO()
apiClient := NewOauth2APIClient(ctx, token)
ctx := context.WithValue(context.TODO(), traq.ContextAccessToken, token.AccessToken)
apiClient := traq.NewAPIClient(traqAPIConfig)
Copy link
Member

Choose a reason for hiding this comment

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

なるほどこれでいいのか
動くならこれでいい気もします
configはグローバルなものを使わずに毎回NewConfiguration()するのがよさそうです

Copy link
Contributor Author

Choose a reason for hiding this comment

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

返信遅くなりました 🙇
確認お願いします

@Nzt3-gh
Copy link
Contributor

Nzt3-gh commented Sep 2, 2024

手元環境で動かすとログインを試みたときに{"errors":{"message":"Internal Server Error"}}となります。
ログは"error": "/app/router/authorization.go:71: 401 Unauthorized" となっているのでtokenの設定が何か違うかもしれないです。

@iChemy
Copy link
Contributor Author

iChemy commented Sep 4, 2024

サーバーのアクセストークンを設定し忘れてたりしませんか?

@Nzt3-gh
Copy link
Contributor

Nzt3-gh commented Sep 5, 2024

解決しました。サーバーのアクセストークンの設定のし忘れでした。
手元での動作は確認できました。

@iChemy
Copy link
Contributor Author

iChemy commented Sep 15, 2024

@Nzt3-gh @ras0q
大変遅くなりました 🙇
ありがとうございます

良さそうだったら approve お願いします

Copy link
Member

@ras0q ras0q left a comment

Choose a reason for hiding this comment

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

見ました、よさそうですありがとうございます🙌

@iChemy iChemy merged commit fc2c812 into main Sep 16, 2024
5 of 6 checks passed
@iChemy iChemy deleted the fix/issue562 branch September 16, 2024 06:31
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.

3 participants