Follow-up from "Add saas feature yml configs"
The following discussions from !133236 (merged) should be addressed:
-
warning showing in MR when it shouldn't !134663 (comment 1611014234) -
@tkuah started a discussion: (+1 comment) A spec is good enough for now, but it would be better to turn this into a runtime error like we do in
lib/feature/definition.rb
for feature flag YAML validation. Maybe a subsequent MR. -
@godfat-gitlab started a discussion: (+2 comments) What do you think about marking this method private, and provide 3 additional public method,
added_files
,modified_files
, anddeleted_files
to call this method?def added_files files(change_type: :added) end
My thinking is that this helps to clarify what are the available types without looking at the comments (which can potentially get outdated), and it might better catch typo (for a method name rather than an argument to a method).
-
@godfat-gitlab started a discussion: (+2 comments) What do you think about calling it
saas_feature_files_added_or_modified?
or something like that? I was pretty confused about the algorithm until I realized that it's not returning all theadded_files
defined for the regular Dangerfile, but here we actually use the same method name for different purpose. -
@godfat-gitlab started a discussion: (+2 comments) Could it maybe be helpful to also print the error message? Any security risk concern?
-
@godfat-gitlab started a discussion: (+1 comment) We should probably disable this for the whole
danger/
directory in.rubocop.yml
. -
@godfat-gitlab started a discussion: (+2 comments) Even in this case, there might have a line for
group:
in the YAML file but just empty. What do you think about tweaking the condition ordering a bit?mr_line = saas_feature.raw.lines.find_index do |line| line.start_with?('group:') end if mr_line markdown(format(SUGGEST_MR_COMMENT, group: mr_group_label), file: saas_feature.path, line: mr_line.succ) elsif mr_group_label warn %(Consider setting `group: "#{mr_group_label}"` in #{helper.html_link(saas_feature.path)}. #{SEE_DOC}) else warn "Consider setting `group` in #{helper.html_link(saas_feature.path)}. #{SEE_DOC}" end
-
@godfat-gitlab started a discussion: (+3 comments) @dstull Sorry that I left quite a lot of comments. Most of them are minor naming things, moving things around, and small code tweaks. The most important one is probably that I think
message_for_group!
wasn't called, and it looks like we do want to check that. -
@group_9970_bot_58e69a7d2ace73c7373ac2bc2a5cabd0 started a discussion: (+1 comment) Consider removing this inline disabling and adhering to the rubocop rule. If that isn't possible, please provide context as a reply for reviewers. See rubocop best practices.