Agreed Ryan, I know this is a bit of a cart before the horse scenario. This isn't the most desirable of circumstances, and we're doing our best to accommodate the contribution because it's a good feature to have, and the work so far looks pretty solid. We have a bit more flexibility in a feature branch than we would if this was (as it was originally) submitted against master - I think the most important thing here is we do architecture/code/test review on an individual PR basis. They've split up this work pretty fine-grained, so I don't think it should be too hard for us to work through. When we get through PRs 1-14 from a code review perspective, we'll have to do a run of manual acceptance tests on the final product before accepting the feature branch into master. Anything that appears to be broken or missing will simply need another PR to address it, and we can get it in. I actually don't think this should be too bad. Again, thank you to Shane, Tibor, and Tamas for breaking this down. Just be careful in the future when you're collaborating on something of this size that you either A) split off the work in fully functioning PRs, e.g. refactoring work or library changes can very easily be submitted against master without breaking anything or B) start with a feature branch and discuss thread that lays out your plan for delivering the feature in that FB.
Please review Apache's guide on consensus building, as well - http://community.apache.org/committers/consensusBuilding.html On Tue, Jun 11, 2019 at 12:01 PM Ryan Merriman <merrim...@gmail.com> wrote: > "We planning to add the changes to the latest PR as additional commits to > avoid impacting the PR sequence. We will refer to the source PR in the > commit message of the fix. Also adding a link to the comment section of the > source PR of the change request to the fixing commit to make them > connected." > > I don't think this is going to work. If changes are requested and applied > in a different PR, how would you test the original PR? What would you do > if there were merge conflicts introduced between the current and final PR? > What's the point of even having any PRs before the final one if they won't > get committed or changed? I don't see any way to avoid having to propagate > changes through all subsequent PRs. > > On Wed, May 29, 2019 at 7:03 AM Tibor Meller <tibor.mel...@gmail.com> > wrote: > > > Hi all, > > > > *We still need some volunteer reviewers for this feature.* All individual > > PR is under 1000 line of changes except one and it is due to an > > autogenerated package.lock.json file. > > Just a heads up: parser aggregation by default turned on for bro, snort > and > > yarn parser on full dev. Without this changeset, full dev is broken. > > > > > https://lists.apache.org/thread.html/beeb4cfddfca7958a22ab926f72f52f46a33c42edce714112df9a2da@%3Cdev.metron.apache.org%3E > > > > > > > > On Fri, May 24, 2019 at 3:20 PM Tibor Meller <tibor.mel...@gmail.com> > > wrote: > > > > > Please find below the list of the PRs we opened for Parser Aggregation. > > > With Shane, Tamas we tried to provide as much information as possible > to > > > make the reviewing process easier. > > > Please keep that in mind these PRs are not against muster but a Parser > > > Aggregation feature branch. > > > If you like to read more about the process we followed with these PRs > > > please read the previous three message in this thread. > > > > > > PR#1 METRON-2114: [UI] Moving components to sensor parser module > > > <https://github.com/apache/metron/pull/1410> > > > PR#2 METRON-2116: [UI] Removing redundant AppConfigService > > > <https://github.com/apache/metron/pull/1411> > > > PR#3 METRON-2117: [UI] Aligning models to grouping feature > > > <https://github.com/apache/metron/pull/1412> > > > PR#4 METRON-2115: [UI] Aligning UI to the parser aggregation AP > > > <https://github.com/apache/metron/pull/1414> > > > PR#5 METRON-2122: [UI] Fixing early app config access issue > > > <https://github.com/apache/metron/pull/1415> > > > PR#6 METRON-2124: [UI] Move status information and start/stop to the > > > Aggregate level <https://github.com/apache/metron/pull/1418> > > > PR#7 METRON-2125: [UI] Making changes visible in the parser list by > > > marking changed items <https://github.com/apache/metron/pull/1422> > > > PR#8 METRON-2131: Add NgRx and related dependencies > > > <https://github.com/apache/metron/pull/1423> > > > PR#9 METRON-2133: Add NgRx effects to communicate with the server > > > <https://github.com/apache/metron/pull/1424> > > > PR#10 METRON-2134: Add NgRx reducers to perform parser and group > changes > > > in the store <https://github.com/apache/metron/pull/1425> > > > PR#11 METRON-2135: Add NgRx actions to trigger state changes > > > <https://github.com/apache/metron/pull/1426> > > > PR#12 METRON-2136: Add parser aggregation sidebar > > > <https://github.com/apache/metron/pull/1427> > > > PR#13 METRON-2137: Implement drag and drop mechanism and wire NgRx > > > <https://github.com/apache/metron/pull/1428> > > > PR#14 METRON-2138: Code clean up > > > <https://github.com/apache/metron/pull/1429> > > > PR#15 METRON-2139: Refactoring sensor-parser-config.component and wire > > > NgRx <https://github.com/apache/metron/pull/1430> > > > > > > Thanks, > > > Tibor > > > > > > > > > On Thu, May 23, 2019 at 11:45 AM Tibor Meller <tibor.mel...@gmail.com> > > > wrote: > > > > > >> Yes, am expecting that some change request will rase due to the > review. > > >> We planning to add the changes to the latest PR as additional commits > to > > >> avoid impacting the PR sequence. We will refer to the source PR in the > > >> commit message of the fix. Also adding a link to the comment section > of > > the > > >> source PR of the change request to the fixing commit to make them > > connected. > > >> > > >> On Wed, May 22, 2019 at 5:49 PM Michael Miklavcic < > > >> michael.miklav...@gmail.com> wrote: > > >> > > >>> Tibor, that sounds reasonable to me. If PR #1 ends up requiring code > > >>> changes, will you guys just percolate those up through the remaining > k > > >>> PRs > > >>> in order, or just the final PR? I'm wondering how this works in > > reference > > >>> to your last point in #5 about rebasing. > > >>> > > >>> On Wed, May 22, 2019 at 8:47 AM Tibor Meller <tibor.mel...@gmail.com > > > > >>> wrote: > > >>> > > >>> > I would like to describe quickly *our approach to breaking down > > Parser > > >>> > Aggregation PR for smaller chunks* > > >>> > > > >>> > *1. we squashed the commits in the original development branch* > > >>> > - when we started to open smaller PRs from the commits from the > > >>> original > > >>> > branch, we found ourself opening PRs out of historical states of > the > > >>> code > > >>> > instead of the final one > > >>> > - none of those states of development are worth (or make sense) to > be > > >>> > reviewed (initial phases of development are included in the > original > > >>> commit > > >>> > history, multiple iterations of refactoring, etc.) > > >>> > - while the actual development history was irrelevant, the > > attribution > > >>> > aspect of it was still important > > >>> > *2. we divided** the changes by the original authors* > > >>> > - the original contributors were sardell, ruffle1986 and tiborm > > >>> > - we isolated the changes that belong to each individual > contributor > > >>> > *3. each of us identified smaller but belonging changesets * > > >>> > - with this, we ended up opening 5 PRs from tiborm, 3 from sardell > > and > > >>> 6 > > >>> > from ruffle1986 > > >>> > - each of these are smaller than 500 lines of changes, which makes > > the > > >>> task > > >>> > of reviewing easier > > >>> > - they have their own context and purpose described by the PR and > the > > >>> > related Jira ticket > > >>> > *4. Each PR introduces a single new commit which is meant to be > > >>> reviewed* > > >>> > - with this we were able to open PRs on top of each others work, > but > > >>> the > > >>> > reviewer is still able to identify what changes were introduced and > > >>> > described by the pr simply by focusing on the last commit > > >>> > - the commit introduced by the PR has the same commit message as > the > > >>> title > > >>> > of the PR to make it easier to find > > >>> > *5. Only the last PR is meant to be merged into the feature branch* > > >>> > - the last PR also introduces a single new commit to being reviewed > > >>> > - this contains all the commits from the previous PRs that belong > to > > >>> parser > > >>> > aggregation > > >>> > - it builds fine in Travis > > >>> > - it's fully functional and ready to being tested against full dev > > >>> > - If we only merge the last PR, we don't have to rebase and > recreate > > >>> all of > > >>> > our PRs due to merge conflicts that will result from to conflicting > > >>> > histories (which is common in feature branch work) > > >>> > > > >>> > Once all the Pull Requests are open, I will submit a list of all of > > >>> them to > > >>> > this discussion thread. > > >>> > > > >>> > On Wed, May 8, 2019 at 3:58 PM Otto Fowler < > ottobackwa...@gmail.com> > > >>> > wrote: > > >>> > > > >>> > > You need to be a committer, that is all I think. > > >>> > > I would not use the github UI for it though, I do it through the > > cli > > >>> > > > > >>> > > > > >>> > > > > >>> > > On May 8, 2019 at 09:45:24, Michael Miklavcic ( > > >>> > michael.miklav...@gmail.com > > >>> > > ) > > >>> > > wrote: > > >>> > > > > >>> > > Not that I'm aware of. Nick and Otto, you've created them before, > > >>> did you > > >>> > > need any special perms? > > >>> > > > > >>> > > On Wed, May 8, 2019 at 3:57 AM Shane Ardell < > > >>> shane.m.ard...@gmail.com> > > >>> > > wrote: > > >>> > > > > >>> > > > This morning, we started to break down our work as Michael > > >>> suggested in > > >>> > > > this thread. However, it looks like I don't have permission to > > >>> create a > > >>> > > new > > >>> > > > branch in the GitHub UI or push a new branch to the > apache/metron > > >>> repo. > > >>> > > Is > > >>> > > > this action restricted to PMC members only? > > >>> > > > > > >>> > > > Shane > > >>> > > > > > >>> > > > On Wed, May 8, 2019 at 9:06 AM Tamás Fodor < > > ftamas.m...@gmail.com> > > >>> > > wrote: > > >>> > > > > > >>> > > > > Here's the process we've gone through in order to implement > the > > >>> > > feature. > > >>> > > > > > > >>> > > > > At the beginning we had some bootstrap work like creating a > > mock > > >>> API > > >>> > > > > (written in NodeJS) because we were a few steps ahead the > > backend > > >>> > part. > > >>> > > > But > > >>> > > > > this is in a totally different repository so it doesn't > really > > >>> count. > > >>> > > We > > >>> > > > > also had to wire NgRX, our chosen 3rd party that supports the > > >>> flux > > >>> > flow > > >>> > > > to > > >>> > > > > get a better state management. When we were ready to kick off > > >>> > > > implementing > > >>> > > > > the business logic in, we splited up the work by subfeatures > > like > > >>> > drag > > >>> > > > and > > >>> > > > > dropping table rows. At this point, we created a POC without > > NgRX > > >>> > just > > >>> > > to > > >>> > > > > let you have the feeling of how it works in real life. Later > > on, > > >>> > after > > >>> > > > > introducing NgRX, we had to refactor it a little bit > obviously > > to > > >>> > make > > >>> > > it > > >>> > > > > compatible with NgRX. There were other subfeatures like > > creating > > >>> and > > >>> > > > > editing groups in a floating pane on the right side of the > > >>> window. > > >>> > > > > When the real backend API was ready we made the necessary > > >>> changes and > > >>> > > > > tested whether it worked how it was expected. There were a > few > > >>> > > difference > > >>> > > > > between how we originally planned the API and the current > > >>> > > implementation > > >>> > > > so > > >>> > > > > we had to adapt it accordingly. While we were implementing > the > > >>> > > features, > > >>> > > > we > > >>> > > > > wrote the unit tests simultaneously. The latest task on the > > >>> feature > > >>> > was > > >>> > > > > restricting the user from aggregating parsers together. > > >>> > > > > > > >>> > > > > As a first iteration, we've decided to put the restriction in > > >>> because > > >>> > > it > > >>> > > > > requires a bigger effort on the backend to deal with that. In > > my > > >>> > > opinion, > > >>> > > > > we should get rid of the restriction because it's not > intuitive > > >>> and > > >>> > > very > > >>> > > > > inconvenient. In my opinion, we should let the users to > > >>> aggregate the > > >>> > > > > running parsers together and do the job to handle this edge > > case > > >>> on > > >>> > the > > >>> > > > > backend accordingly. > > >>> > > > > > > >>> > > > > What do you think, guys? > > >>> > > > > > > >>> > > > > Hope this helps. > > >>> > > > > > > >>> > > > > Tamas > > >>> > > > > > > >>> > > > > On Tue, May 7, 2019 at 4:34 PM Michael Miklavcic < > > >>> > > > > michael.miklav...@gmail.com> wrote: > > >>> > > > > > > >>> > > > > > This was my expectation as well. > > >>> > > > > > > > >>> > > > > > Shane, Tibor, Tamas - how did you go about breaking this > down > > >>> into > > >>> > > > chunks > > >>> > > > > > and/or microchunks when you collaborated offline? As Nick > > >>> > mentioned, > > >>> > > > you > > >>> > > > > > obviously split up work and shared it amongst yourselves. > > Some > > >>> > > > > explanation > > >>> > > > > > around this process would be helpful for reviewers as well. > > We > > >>> > might > > >>> > > be > > >>> > > > > > able to provide better guidance and examples to future > > >>> contributors > > >>> > > as > > >>> > > > > > well. > > >>> > > > > > > > >>> > > > > > I talked a little bit with Shane about this offline last > > week. > > >>> It > > >>> > > looks > > >>> > > > > > like you guys effectively ran a local feature branch. > > >>> Replicating > > >>> > > that > > >>> > > > > > process in a feature branch in Apache is probably what you > > guys > > >>> > > should > > >>> > > > > > be doing for a change this size. We don't have hard limits > on > > >>> line > > >>> > > > change > > >>> > > > > > size, but in the past it's been somewhere around 2k-3k > lines > > >>> and > > >>> > > above > > >>> > > > > > being the tipping point for discussing a feature branch. > > >>> Strictly > > >>> > > > > speaking, > > >>> > > > > > line quantity alone is not the only metric, but it's > relevant > > >>> here. > > >>> > > If > > >>> > > > > you > > >>> > > > > > want to make smaller incremental changes locally, there's > > >>> nothing > > >>> > to > > >>> > > > keep > > >>> > > > > > you from doing that - I would only advise that you consider > > >>> > squashing > > >>> > > > > those > > >>> > > > > > commits (just ask if you're unclear about how to handle > that) > > >>> into > > >>> > a > > >>> > > > > single > > >>> > > > > > larger commit/chunk when you're ready to publish them as a > > >>> chunk to > > >>> > > the > > >>> > > > > > public feature branch. So it would look something like > this: > > >>> > > > > > > > >>> > > > > > Commits by person locally > > >>> > > > > > Shane: 1,2,3 -> squash as A > > >>> > > > > > Tibor: 4,5,6 -> squash as B > > >>> > > > > > Tamas: 7,8,9 -> squash as C > > >>> > > > > > > > >>> > > > > > Commits by person in Apache > > >>> > > > > > Shane: A > > >>> > > > > > Tibor: B > > >>> > > > > > Tamas: C > > >>> > > > > > > > >>> > > > > > We need to maintain a record of attribution. Your real > > >>> workflow may > > >>> > > not > > >>> > > > > be > > >>> > > > > > that cleanly delineated, but you can choose how you want to > > >>> squash > > >>> > in > > >>> > > > > those > > >>> > > > > > cases. Even in public collaboration, there are plenty of > > cases > > >>> > where > > >>> > > > > folks > > >>> > > > > > submit PRs against PRs, abstain from accepting attribution, > > >>> and it > > >>> > > all > > >>> > > > > gets > > >>> > > > > > squashed down into one person's final PR commit. There are > > many > > >>> > > > options. > > >>> > > > > > > > >>> > > > > > Hope this helps. > > >>> > > > > > > > >>> > > > > > Best, > > >>> > > > > > Mike > > >>> > > > > > > > >>> > > > > > On Mon, May 6, 2019 at 8:19 AM Nick Allen < > > n...@nickallen.org> > > >>> > > wrote: > > >>> > > > > > > > >>> > > > > > > Have you considered creating a feature branch for the > > effort? > > >>> > This > > >>> > > > > would > > >>> > > > > > > allow you to break the effort into chunks, where the > result > > >>> of > > >>> > each > > >>> > > > PR > > >>> > > > > > may > > >>> > > > > > > not be a fully working "master-ready" result. > > >>> > > > > > > > > >>> > > > > > > I am sure you guys tackled the work in chunks when > > >>> developing it, > > >>> > > so > > >>> > > > > > > consider just replaying those chunks onto the feature > > branch > > >>> as > > >>> > > > > separate > > >>> > > > > > > PRs. > > >>> > > > > > > > > >>> > > > > > > > > >>> > > > > > > > > >>> > > > > > > On Mon, May 6, 2019 at 5:24 AM Tibor Meller < > > >>> > > tibor.mel...@gmail.com> > > >>> > > > > >>> > > > > > > wrote: > > >>> > > > > > > > > >>> > > > > > > > I wondered on the weekend how we could split that PR to > > >>> smaller > > >>> > > > > chunks. > > >>> > > > > > > > That PR is a result of almost 2 months of development > > and I > > >>> > don't > > >>> > > > see > > >>> > > > > > how > > >>> > > > > > > > to split that to multiple WORKING parts. It is as it > is a > > >>> whole > > >>> > > > > working > > >>> > > > > > > > feature. If we split it by packages or files we could > > >>> provide > > >>> > > > smaller > > >>> > > > > > > > non-functional PR's, but can end up having a broken > > >>> Management > > >>> > UI > > >>> > > > > after > > >>> > > > > > > > having the 1st PR part merged into master. I don't > think > > >>> that > > >>> > > would > > >>> > > > > be > > >>> > > > > > > > acceptable by the community (or even by me) so I would > > >>> like to > > >>> > > > > suggest > > >>> > > > > > > two > > >>> > > > > > > > other option to help review PR#1360. > > >>> > > > > > > > > > >>> > > > > > > > #1 We could extend that PR with our own author comments > > in > > >>> > > Github. > > >>> > > > > That > > >>> > > > > > > > would help following which code part belongs to where > and > > >>> why > > >>> > it > > >>> > > > was > > >>> > > > > > > > necessary. > > >>> > > > > > > > #2 We can schedule an interactive code walkthrough call > > >>> with > > >>> > the > > >>> > > > ones > > >>> > > > > > who > > >>> > > > > > > > interested in reviewing or the particular changeset. > > >>> > > > > > > > > > >>> > > > > > > > Please share your thoughts on this! Which version would > > >>> support > > >>> > > you > > >>> > > > > the > > >>> > > > > > > > best? Or if you have any other idea let us know. > > >>> > > > > > > > > > >>> > > > > > > > PS: I think the size of our PR's depends on how small > > >>> > > independently > > >>> > > > > > > > deliverable changesets we can identify before we > starting > > >>> to > > >>> > > > > implement > > >>> > > > > > a > > >>> > > > > > > > relatively big new feature. Unfortunately, we missed to > > do > > >>> that > > >>> > > > with > > >>> > > > > > this > > >>> > > > > > > > feature. > > >>> > > > > > > > > > >>> > > > > > > > On Fri, May 3, 2019 at 1:49 PM Shane Ardell < > > >>> > > > > shane.m.ard...@gmail.com> > > >>> > > > > > > > wrote: > > >>> > > > > > > > > > >>> > > > > > > > > NgRx was only used for the aggregation feature and > > >>> doesn't go > > >>> > > > > beyond > > >>> > > > > > > > that. > > >>> > > > > > > > > I think the way I worded that sentence may have > caused > > >>> > > > confusion. I > > >>> > > > > > > just > > >>> > > > > > > > > meant we use it to manage more pieces of state within > > the > > >>> > > > > aggregation > > >>> > > > > > > > > feature than just previous and current state of > grouped > > >>> > > parsers. > > >>> > > > > > > > > > > >>> > > > > > > > > On Fri, May 3, 2019 at 1:32 AM Michael Miklavcic < > > >>> > > > > > > > > michael.miklav...@gmail.com> wrote: > > >>> > > > > > > > > > > >>> > > > > > > > > > Shane, thanks for putting this together. The > updates > > >>> on the > > >>> > > > Jira > > >>> > > > > > are > > >>> > > > > > > > > useful > > >>> > > > > > > > > > as well. > > >>> > > > > > > > > > > > >>> > > > > > > > > > > (we used it for more than just that in this > > feature, > > >>> but > > >>> > > that > > >>> > > > > was > > >>> > > > > > > the > > >>> > > > > > > > > > initial reasoning) > > >>> > > > > > > > > > What are you using NgRx for in the submitted work > > that > > >>> goes > > >>> > > > > beyond > > >>> > > > > > > the > > >>> > > > > > > > > > aggregation feature? > > >>> > > > > > > > > > > > >>> > > > > > > > > > > > >>> > > > > > > > > > > > >>> > > > > > > > > > On Thu, May 2, 2019 at 12:22 PM Shane Ardell < > > >>> > > > > > > shane.m.ard...@gmail.com > > >>> > > > > > > > > > > >>> > > > > > > > > > wrote: > > >>> > > > > > > > > > > > >>> > > > > > > > > > > Hello everyone, > > >>> > > > > > > > > > > > > >>> > > > > > > > > > > In response to discussions in the 0.7.1 release > > >>> thread, I > > >>> > > > > wanted > > >>> > > > > > to > > >>> > > > > > > > > > start a > > >>> > > > > > > > > > > thread regarding the parser aggregation work for > > the > > >>> > > > Management > > >>> > > > > > UI. > > >>> > > > > > > > For > > >>> > > > > > > > > > > anyone who has not already read and tested the PR > > >>> > locally, > > >>> > > > I've > > >>> > > > > > > > added a > > >>> > > > > > > > > > > detailed description of what we did and why to > the > > >>> JIRA > > >>> > > > ticket > > >>> > > > > > > here: > > >>> > > > > > > > > > > > https://issues.apache.org/jira/browse/METRON-1856 > > >>> > > > > > > > > > > > > >>> > > > > > > > > > > I'm wondering what the community thinks about > what > > >>> we've > > >>> > > > built > > >>> > > > > > thus > > >>> > > > > > > > > far. > > >>> > > > > > > > > > Do > > >>> > > > > > > > > > > you see anything missing that must be part of > this > > >>> new > > >>> > > > feature > > >>> > > > > in > > >>> > > > > > > the > > >>> > > > > > > > > UI? > > >>> > > > > > > > > > > Are there any strong objections to how we > > >>> implemented it? > > >>> > > > > > > > > > > > > >>> > > > > > > > > > > I’m also looking to see if anyone has any > thoughts > > >>> on how > > >>> > > we > > >>> > > > > can > > >>> > > > > > > > > possibly > > >>> > > > > > > > > > > simplify this PR. Right now it's pretty big, and > > >>> there > > >>> > are > > >>> > > a > > >>> > > > > lot > > >>> > > > > > of > > >>> > > > > > > > > > commits > > >>> > > > > > > > > > > to parse through, but I'm not sure how we could > > break > > >>> > this > > >>> > > > work > > >>> > > > > > out > > >>> > > > > > > > > into > > >>> > > > > > > > > > > separate, smaller PRs opened against master. We > > >>> could try > > >>> > > to > > >>> > > > > > > > > cherry-pick > > >>> > > > > > > > > > > the commits into smaller PRs and then merge them > > >>> into a > > >>> > > > feature > > >>> > > > > > > > branch, > > >>> > > > > > > > > > but > > >>> > > > > > > > > > > I'm not sure if that's worth the effort since > that > > >>> will > > >>> > > only > > >>> > > > > > reduce > > >>> > > > > > > > the > > >>> > > > > > > > > > > number commits to review, not the lines changed. > > >>> > > > > > > > > > > > > >>> > > > > > > > > > > As an aside, I also want to give a little > > background > > >>> into > > >>> > > the > > >>> > > > > > > > > > introduction > > >>> > > > > > > > > > > of NgRx in this PR. To give a little background > on > > >>> why we > > >>> > > > chose > > >>> > > > > > to > > >>> > > > > > > do > > >>> > > > > > > > > > this, > > >>> > > > > > > > > > > you can refer to the discussion thread here: > > >>> > > > > > > > > > > > > >>> > > > > > > > > > > > > >>> > > > > > > > > > > > >>> > > > > > > > > > > >>> > > > > > > > > > >>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > > >>> > > > >>> > > > https://lists.apache.org/thread.html/06a59ea42e8d9a9dea5f90aab4011e44434555f8b7f3cf21297c7c87@%3Cdev.metron.apache.org%3E > > >>> > > > > > > > > > > > > >>> > > > > > > > > > > We previously discussed introducing a better way > to > > >>> > manage > > >>> > > > > > > > application > > >>> > > > > > > > > > > state in both UIs in that thread. It was decided > > that > > >>> > NgRx > > >>> > > > was > > >>> > > > > a > > >>> > > > > > > > great > > >>> > > > > > > > > > tool > > >>> > > > > > > > > > > for many reasons, one of them being that we can > > >>> piecemeal > > >>> > > it > > >>> > > > > into > > >>> > > > > > > the > > >>> > > > > > > > > > > application rather than doing a huge rewrite of > all > > >>> the > > >>> > > > > > application > > >>> > > > > > > > > state > > >>> > > > > > > > > > > at once. The contributors in this PR (myself > > >>> included) > > >>> > > > decided > > >>> > > > > > this > > >>> > > > > > > > > would > > >>> > > > > > > > > > > be a perfect opportunity to introduce NgRx into > the > > >>> > > > Management > > >>> > > > > UI > > >>> > > > > > > > since > > >>> > > > > > > > > > we > > >>> > > > > > > > > > > need to manage the previous and current state > with > > >>> the > > >>> > > > grouping > > >>> > > > > > > > feature > > >>> > > > > > > > > > so > > >>> > > > > > > > > > > that users can undo the changes they've made (we > > >>> used it > > >>> > > for > > >>> > > > > more > > >>> > > > > > > > than > > >>> > > > > > > > > > just > > >>> > > > > > > > > > > that in this feature, but that was the initial > > >>> > reasoning). > > >>> > > In > > >>> > > > > > > > addition, > > >>> > > > > > > > > > we > > >>> > > > > > > > > > > greatly benefited from this when it came time to > > >>> debug > > >>> > our > > >>> > > > work > > >>> > > > > > in > > >>> > > > > > > > the > > >>> > > > > > > > > UI > > >>> > > > > > > > > > > (the discussion in the above thread link goes a > > >>> little > > >>> > more > > >>> > > > > into > > >>> > > > > > > the > > >>> > > > > > > > > > > advantages of debugging with NgRx and DevTools). > > >>> Removing > > >>> > > > NgRx > > >>> > > > > > from > > >>> > > > > > > > > this > > >>> > > > > > > > > > > work would reduce the numbers of lines changed > > >>> slightly, > > >>> > > but > > >>> > > > it > > >>> > > > > > > would > > >>> > > > > > > > > > still > > >>> > > > > > > > > > > be a big PR and a lot of that code would just > move > > >>> to the > > >>> > > > > > component > > >>> > > > > > > > or > > >>> > > > > > > > > > > service level in the Angular application. > > >>> > > > > > > > > > > > > >>> > > > > > > > > > > Shane > > >>> > > > > > > > > > > > > >>> > > > > > > > > > > > >>> > > > > > > > > > > >>> > > > > > > > > > >>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > > >>> > > >> > > >
