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.

Sign up for GitHub

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

Jump to bottom

設定ファイルから設定値を読み込む機能の実装 #6

Merged
merged 4 commits into from
May 14, 2021
Merged

設定ファイルから設定値を読み込む機能の実装 #6

merged 4 commits into from
May 14, 2021

Conversation

Sibakeny
Copy link
Contributor

@Sibakeny Sibakeny commented May 7, 2021 •
edited
Loading

📎 関連リンク

仕様Growi
https://mcloud.growi.cloud/MCOSS/%E8%A8%AD%E5%AE%9A%E5%80%A4%E8%AA%AD%E3%81%BF%E5%8F%96%E3%82%8A%E6%A9%9F%E8%83%BD

✅ やったこと

  • フォルダ構成の修正
    • Sp::Rails::Samlの様にそれぞれmoduleで分けられていたのでSpRailsSamlのmoduleを使用する形に修正し、それに伴いフォルダ構成をsp/rails/samlからsp_rails_samlに変更しました。
  • Settingsクラスの作成(テスト実装時はSettingクラスで実装していたのですが、ruby-samlに合わせてSettingsクラスに変更しました)
  • setupメソッドにて設定値を保存する機能の実装
  • 設定ファイルを自動生成する機能の実装

実際にrailsプロジェクトに導入して動作確認した際の内容

# 設定ファイルの自動生成
$ rails generate sp_rails_saml:config
Running via Spring preloader in process 61435
      create  config/initializers/sp-rails-saml.rb

# 設定値が読み込めているか確認
$ rails c
Running via Spring preloader in process 61457
Loading development environment (Rails 6.1.3)
irb(main):001:0> SpRailsSaml::Settings.name_identifier_format
=> "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress"

📝 やらなかったこと

👀 確認したい内容

  • コードの書き方で改善点がないかどうか

@Sibakeny Sibakeny changed the base branch from develop to milestone/saml_setting_initializer May 7, 2021 08:55
@Sibakeny Sibakeny force-pushed the create_saml_settings_class branch from 8a5849e to 7e1fceb Compare May 7, 2021 08:56
@Sibakeny Sibakeny force-pushed the create_saml_settings_class branch 2 times, most recently from 46a53af to ca96d11 Compare May 10, 2021 05:27
@Sibakeny Sibakeny force-pushed the create_saml_settings_class branch from ca96d11 to a17ce0e Compare May 10, 2021 05:28
@Sibakeny
Copy link
Contributor Author

仕様策定とテストの実装のPRにて想定漏れがありましたので、そちらに関して仕様の作成とテストの実装をこのPRで一緒に対応しました。

内容としてはgeneratorのコマンドを叩いた際に、設定ファイルをconfig/initializersフォルダ内に作成するといった内容です。

今回のPRにて、下記のコマンドにて設定ファイルが自動生成される形で実装を行いました。

rails generate sp_rails_saml:config

実装に関してのレビューと合わせて、上記の形で設定ファイルが自動生成されるといった仕様に関して問題ないかも確認していただけるとありがたいです！

@Sibakeny Sibakeny requested a review from naomichi-y May 10, 2021 05:46
@Sibakeny Sibakeny changed the title 設定ファイルから設定値を保存するためのSettingsクラスの作成 設定ファイルから設定値を読み込む機能の実装 May 10, 2021
naomichi-y
naomichi-y approved these changes May 14, 2021
View reviewed changes
lib/generators/sp-rails-saml/config_generator.rb
@@ -0,0 +1,26 @@
require 'rails/generators'
Copy link
Member

Choose a reason for hiding this comment

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

文字列を括る際にシングルクォート、ダブルクォートが混ざってますが、この辺りはrubocopなどで統一 (一括で統一) できます。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ありがとうございます！
別途rubocopでコードを綺麗にする対応を行いたいと思います！

lib/sp-rails-saml.rb
require 'generators/sp-rails-saml/config_generator'

module SpRailsSaml
class Error < StandardError; end
Copy link
Member

Choose a reason for hiding this comment

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

ライブラリ内で発生するエラークラスは SpRailsSaml:Error で統一するか、あるいユースケース単位でサブクラスを定義するべきか方針を決めたほうが良いかと思います。

e.g.
https://github.com/metaps/genova/blob/develop/lib/genova/exceptions.rb#L3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ありがとうございます！
別途検討したいと思います！
#17

spec/sp_rails_saml/saml_spec.rb
@@ -0,0 +1,5 @@
RSpec.describe SpRailsSaml do
it "has a version number" do
Copy link
Member

Choose a reason for hiding this comment

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

テストコードはどの粒度 (カバレッジやテスト観点など) で作成するか方針は決まってますか?
まだであればガイドラインを策定しておいたほうが良いかと思います。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

別タスクで方針を決めたいと思います！
#16

This was referenced May 14, 2021
Open
Closed
@Sibakeny Sibakeny merged commit 86e1acc into milestone/saml_setting_initializer May 14, 2021
@Sibakeny Sibakeny mentioned this pull request May 14, 2021
Merged
2 tasks
Sibakeny added a commit that referenced this pull request May 14, 2021
@Sibakeny
[milestone] samlの設定を設定ファイルから読み込む (#4)
74f0f87 
* settingクラスに関するテストの作成 (#2)

Co-authored-by: yonetani <[email protected]>

* 設定ファイルから設定値を読み込む機能の実装 (#6)

* フォルダの配置の修正

* Settingsクラスの作成

* 設定値を読み込む機能の実装

* 不要なrequire削除

Co-authored-by: yonetani <[email protected]>

* x86_64-darwinのプラットフォームでnokogiriをインストールすることでciが失敗していたので修正

Co-authored-by: yonetani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@naomichi-y naomichi-y naomichi-y approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Sibakeny @naomichi-y