-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENG 3008 Fix registered with no run workflows #1374
ENG 3008 Fix registered with no run workflows #1374
Conversation
…e-enable-engine-on-registered-workflow
@@ -191,7 +215,6 @@ const WorkflowsPage: React.FC<Props> = ({ user, Layout = DefaultLayout }) => { | |||
skip: dagResultsLoading && latestDagId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this is probably wrong as it will not skip if dagResultsLoading
is false but latestDagId
is empty. I'd just use skip: !latestDagId
to load dag whenever the ID is available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Now that things are working let's spend a little time refactoring the codes here:
- just make sure you are on latest main so that you don't see potential large merge conflicts from recent updates
- we can move duplicated dag / result getting code to a helper like
useWorkflowLatestDagOrResult()
that takes the wfID, call internally theuseDagResults
anduseDags
, and returns (potentially undefined) the latestdag
anddagResult
objects. In this way the caller should only care when these objects are available, while not need to care how they are fetched and why they are not available:
const { dag, dagResult } = useWorkflowLatestDagOrResult(apiKey, wfID)
const dagID = dag?.id
const dagResultID = dagResult?.id
// do things based on whether dag / dagResut / dagID / dagResultID is available
…e-enable-engine-on-registered-workflow
@@ -74,6 +75,29 @@ export const getArtifactResultTableRow = ( | |||
}; | |||
}; | |||
|
|||
export function getLatestDagResult( | |||
dagResults: DagResultResponse[] | |||
): DagResultResponse { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I slightly prefer return an undefined
here if there's no result available, and let the caller decide how to deal with undefined
result
let latestDagId; | ||
if (!dagResultsLoading && !dagResultsError && dagResults.length > 0) { | ||
const latestDagResult = getLatestDagResult(dagResults); | ||
let latestDagResultId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer using const
if possible: https://stackoverflow.com/questions/41086633/why-most-of-the-time-should-i-use-const-instead-of-let-in-javascript#:~:text=Using%20const%20instead%20of%20let,somewhere%20else%20in%20the%20code.
Here we can use const latestDagResultID = latestDagResult?.id
and const dagId =
latestDagResult?.dag_id ?? dag?.id`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See if this is still relevant after the suggested change in useLatestDagResultOrDag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for re-organizing the code! I have a few nitpicking comments to improve the simplicity, making hooks less caller-dependent, and have better use of const
over let
…e-enable-engine-on-registered-workflow
…e-enable-engine-on-registered-workflow
Describe your changes and why you are making these changes
If there is no run, there is no dagResult to get the dag id from. Set the latest dag to the 0-th dag in workflow dags if there is no dag result.
Related issue number (if any)
ENG 3008
Loom demo (if any)
Checklist before requesting a review
python3 scripts/run_linters.py -h
for usage).run_integration_test
: Runs integration testsskip_integration_test
: Skips integration tests (Should be used when changes are ONLY documentation/UI)