> On Aug. 14, 2015, 3:54 p.m., David McLaughlin wrote: > > src/main/resources/scheduler/assets/js/controllers.js, line 606 > > <https://reviews.apache.org/r/37365/diff/7/?file=1040096#file1040096line606> > > > > You can just do BaseJobController($scope, ...) now? Here and in other > > invocations. > > Joshua Cohen wrote: > This is pretty standard mechanism for inheritance in JS. We could remove > the .call invocation, but then if something changed in the future that *did* > bind to `this` in the parent it wouldn't work properly. I'd prefer to keep it > as is.
So maybe I don't understand why you're using classical inheritance here then, over a vanilla function? > On Aug. 14, 2015, 3:54 p.m., David McLaughlin wrote: > > src/main/resources/scheduler/assets/js/app.js, lines 35-38 > > <https://reviews.apache.org/r/37365/diff/7/?file=1040095#file1040095line35> > > > > I feel like changing our URLs like this needs a deprecation path. This > > would break tooling around Aurora that assumes how to construct update > > URLs, as seen by the fix you had to apply to the client code. > > Joshua Cohen wrote: > Yeah, I realize this couples a scheduler and client release. I could try > and detect instance ids that seem like guids and redirect? That way as long > as the scheduler is released before the client it should work in both > scenarios. Do you think that would be sufficient? Sounds good. It's a shame angular's default router doesn't allow regexes, as you could have just had a \d+ regex qualifier on instance ids and sent anything else to the update controller for backwards compatibility. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37365/#review95427 ----------------------------------------------------------- On Aug. 14, 2015, 2:44 p.m., Joshua Cohen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37365/ > ----------------------------------------------------------- > > (Updated Aug. 14, 2015, 2:44 p.m.) > > > Review request for Aurora, David McLaughlin and Maxim Khutornenko. > > > Repository: aurora > > > Description > ------- > > Add a new UI page to show all tasks (active and completed) for a specific > instance id. > > N.B.: changes to controllers.js look significant, but it's 90% refactoring so > that JobController and JobInstanceController can reuse common code. > > > Diffs > ----- > > src/main/python/apache/aurora/client/base.py > dee4c28a26828912bec073c7e5213a03c9703eee > src/main/resources/scheduler/assets/breadcrumb.html > 13147935c3c28680d5decd1e03e40d9cf04aaed3 > src/main/resources/scheduler/assets/css/app.css > a4735efe58a09a46f2b95ebe98f219cb9e7ee27d > src/main/resources/scheduler/assets/instance.html PRE-CREATION > src/main/resources/scheduler/assets/job.html > 942635a61132bb9a38a6faf3f8fe046e6bcabd95 > src/main/resources/scheduler/assets/js/app.js > b66409f628046c67b62c92a75cbeed20c09b4ec1 > src/main/resources/scheduler/assets/js/controllers.js > 04ea1cb6998af7c238eb2fd1a7d05a9663abc56c > src/main/resources/scheduler/assets/js/services.js > a514fa7da3c049e1f5a3e49263a82cf2c28d1b2d > src/main/resources/scheduler/assets/taskInstance.html PRE-CREATION > src/test/python/apache/aurora/client/cli/test_supdate.py > 21bea702a6e6fe68652e2195c5d24b56b6477738 > src/test/python/apache/aurora/client/test_base.py > 1a560088279ac945cce14d02454e50b8483771e4 > > Diff: https://reviews.apache.org/r/37365/diff/ > > > Testing > ------- > > - ./gradlew jsHint > - ./pants test src/test/python/apache/aurora/client:base > - Verified update page url in client was correct in Vagrant. > - See attached screenshot for an example of what the instance page looks like. > > > File Attachments > ---------------- > > Example of instance page in action. > > https://reviews.apache.org/media/uploaded/files/2015/08/11/2ab72f9d-9e60-4b86-b971-b6aba5465def__Screen_Shot_2015-08-11_at_4.53.07_PM.png > > > Thanks, > > Joshua Cohen > >