Skip to content
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

hasActiveWorkflow should consider workflows in state PENDING and RUNNING #115

Merged
merged 2 commits into from
Jan 19, 2021

Conversation

splaspood
Copy link
Contributor

Description

When checking for the status of a 'job' boots currently only considers PENDING workflows. This change makes it consider RUNNING workflows to be 'active' as well.

Why is this needed

During workflow execution we pass status updates back to our central API by way of boots. As-is, the hasActiveWorkflow function will reject those status updates as it only considers 'PENDING' workflows to be active.

@codecov
Copy link

codecov bot commented Jan 5, 2021

Codecov Report

Merging #115 (f701c42) into master (b30ab2f) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #115   +/-   ##
=======================================
  Coverage   40.90%   40.90%           
=======================================
  Files          40       40           
  Lines        2056     2056           
=======================================
  Hits          841      841           
  Misses       1130     1130           
  Partials       85       85           
Impacted Files Coverage Δ
job/job.go 34.72% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b30ab2f...f701c42. Read the comment docs.

job/job_test.go Show resolved Hide resolved
kqdeng
kqdeng previously approved these changes Jan 5, 2021
mmlb
mmlb previously approved these changes Jan 5, 2021
Copy link
Contributor

@mmlb mmlb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Cbkhare
Cbkhare previously requested changes Jan 6, 2021
job/job.go Show resolved Hide resolved
Signed-off-by: James W. Brinkerhoff <jwb@paravolve.net>
Signed-off-by: James W. Brinkerhoff <jwb@paravolve.net>
Copy link

@jgavinray jgavinray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@markjacksonfishing markjacksonfishing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@mmlb mmlb dismissed Cbkhare’s stale review January 19, 2021 17:00

Always booting to osie&tink-worker if a workflow is already started is ok.

@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Jan 19, 2021
@mergify mergify bot merged commit 439b17f into tinkerbell:master Jan 19, 2021
@Cbkhare
Copy link
Contributor

Cbkhare commented Jan 29, 2021

@mmlb / @splaspood I have opened a bug with reference to this commit #125.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants