aerie icon indicating copy to clipboard operation
aerie copied to clipboard

feature: Procedural Scheduling

Open skovati opened this issue 1 year ago • 2 comments

Need I say more?

(will fill out later, just looking at CI tests for now)

skovati avatar Jul 08 '24 16:07 skovati

Yep, working on them right now using CI tests

skovati avatar Jul 08 '24 17:07 skovati

@Mythicaeda Ah I didn't push my latest commit with comments / cleanup. Will push when @JoelCourtney is finished force pushing the bug fix

skovati avatar Jul 09 '24 17:07 skovati

I've gone ahead and rebased this branch to clean up the history. The original history is preserved at https://github.com/NASA-AMMOS/aerie/compare/develop...feature/procedural-scheduling--pre-rebase

This link I think is the best summary for what has changed since @Mythicaeda last reviewed. (based on the timestamps, I think 724263c1a is a good reference point). The biggest difference is I split the one database change into two steps: first add goal invocations, and then add the rest of the procedural scheduling machinery. https://github.com/NASA-AMMOS/aerie/compare/724263c1a..feature/procedural-scheduling

mattdailis avatar Aug 07 '24 17:08 mattdailis

Met with @skovati and @mattdailis and we reviewed all of @Mythicaeda 's remaining unresolved comments.

Those marked with a 👍 reaction are considered in scope and either already fixed, or will be on Monday. Those marked with 👀 will be split off into a new future PR & are out of scope for this release - these are mainly related to fleshing out our e2e tests, and while we all agree there are improvements to be made, it's not worth holding up the release of this feature. We'll work on improved tests starting next week.

We noticed that after our latest round of fix commits, the tests are back to being broken - we'll defer release until Monday & @mattdailis will look at fixing these.

@Mythicaeda let me know if you want to tag up on any specifics

dandelany avatar Sep 06 '24 01:09 dandelany

@dandelany I'm specifically concerned with completing a basic test suite being considered "out of scope" for a major feature. The fact that the existing tests have been easy to break via changes this whole PR makes me explicitly concerned that there may be bugs that were added during the review but we're unaware of because the PR was "extensively manually tested" around when it was opened.

I understand that we're trying to get this feature out urgently, which is why I kept the scope of my test requests limited.

Mythicaeda avatar Sep 09 '24 15:09 Mythicaeda