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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
