mmiklavc commented on issue #1431: METRON-2102: [UI] Adding click-through navigation to Alerts table URL: https://github.com/apache/metron/pull/1431#issuecomment-498797705 @tiborm The follow on Jiras make sense. Thanks for responding to these questions and adding the documentation and tests. You probably don't want every edge case validation conditional spelled out in the docs, but I think it's worth letting the user know what to expect, e.g. "Be aware that if you misspell a property, it will be treated as non-existent and the default value of 'X' will be used. You will not get an exception/error." As a general soft rule around adding features, they can typically come in these flavors, in quasi-descending order of desirability: 1. Feature added with full Management UI or Ambari support for enabling/disabling and configuring in single PR. (if it's a huge PR, it may be better to go with option 2) 2. Feature added with UI config support, but split into multiple PRs. Indicate in the description what you're doing and link the PRs to one another in the description. You can request individual review of each PR and say something like "don't merge until all K PRs in this work have been +1'ed," and then commit each PR in sequence. Ideally, the last PR will include all K-1 PRs and will have been tested to ensure the entire unit of work works. See parser chaining as an example. https://github.com/apache/metron/pull/1084. I would take this a step further and explicitly add the links to the PRs. > Note, this PR depends on METRON-1643 and METRON-1642, so those should be reviewed prior to this. 3. Feature added without UI support. **MUST** have documentation for manual configuration. Create Jiras to track the follow-on work and link them to the original. 4. Feature flags - yet to be formally adopted as an approach in Metron, but there is open discussion on it. Not really applicable here, but one other option in situations like this is a feature branch. We typically use that for more disruptive and/or larger changes that we want more flexibility with on the individual commits, but it's worth mentioning. In this case, the gating factor for the individual PRs in the feature branch is a bit lower. You might provide a PR that breaks something temporarily in the FB, for example. Or you save documentation until after you've iterated on the feature enough to be comfortable with the implementation. You indicate what the necessary follow-on work will be arising from the PR against the feature branch, create a sub-task for it on the feature Jira, and it becomes a gating factor for final vote and acceptance on merging the FB into master. This PR fits option 3, which is perfectly fine. We just need a bit of expansion on testing and documentation as we've already touched on. Apache Metron community standards around automated and manual testing apply the same in all cases, with the exception of feature branches where only the final unit of work is reviewed by that standard. Any committer can veto a PR and the approach taken at any time if they think it will not work well for the product and its users, so ymmv. Communicating the full feature delivery lifecycle (e.g. justifying/explaining the why and how of follow-ons) obviously helps grease the skids on this and increases the likelihood of an expedient review and ultimately a +1. Ok, enough on dev guidelines for now. I should probably get these thoughts into a discuss thread and our dev guidelines at some point, but it seemed useful here as we discussed the implementation choices.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
