Improve to add IP addresses export/import to the utility #43
Conversation
| if import_address_with_comments.blank? | ||
| abort 'IP addresses to add must be specified with IMPORT environment variable' | ||
| else | ||
| filter_rule.allowed_ips = ([filter_rule.allowed_ips_with_comments, import_address_with_comments]).join("\n") | ||
| end |
There was a problem hiding this comment.
Hello @takenory . Thank you for the pull request.
In my review at #36 (review), I made a mistake in my suggestion.
Right now, the Import feature adds to the current allowed IP addresses. But, I think it should replace the current allowed IP addresses with the imported ones.
| if import_address_with_comments.blank? | |
| abort 'IP addresses to add must be specified with IMPORT environment variable' | |
| else | |
| filter_rule.allowed_ips = ([filter_rule.allowed_ips_with_comments, import_address_with_comments]).join("\n") | |
| end | |
| if import_address_with_comments.blank? | |
| abort 'IP addresses to import must be specified with IMPORT environment variable' | |
| else | |
| filter_rule.allowed_ips = import_address_with_comments | |
| end |
My suggestion has a downside. Since it's not adding, if setting the allowed IP addresses fails, we would have to run the import again or manually add remote addresses. But, since we can run rake tasks, I think we can handle it.
I would appreciate your feedback on this matter.
There was a problem hiding this comment.
@maeda-m Thx for suggetion.
I thought it was debatable whether the behavior of import should be "replace" or not.
When importing issues from CSV in Redmine, the imported data behaves as "appended", not "replaced".
If the risk is that a successful import with incorrect data will "replace" the original IP address settings (and in some cases make them unrecoverable), I think it is safer to behave as an "addition".
maeda-m
left a comment
maeda-m
left a comment
There was a problem hiding this comment.
LGTM 👍
If we consider the risk of the original IP address settings being "replaced" due to successful import of incorrect data (which might become irrecoverable in some cases), it's safer to act as "addition" rather than replacement.
Also, when importing issues from CSV in Redmine, the imported data behaves as an "addition" rather than a "replacement", so it's less surprising if redmine_ip_filter also follows that behavior.
|
@maeda-m |
2 participants
This PR is to add the export/import that @maeda-m proposed in #42.
Thanks to @maeda-m for his useful suggestion!