Skip to content

Conversation

townsend2010
Copy link
Contributor

Fixes #2170

@codecov
Copy link

codecov bot commented Jul 9, 2021

Codecov Report

Merging #2171 (09d3362) into main (e55cb8e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2171   +/-   ##
=======================================
  Coverage   82.61%   82.61%           
=======================================
  Files         186      186           
  Lines        9632     9637    +5     
=======================================
+ Hits         7957     7962    +5     
  Misses       1675     1675           
Impacted Files Coverage Δ
src/daemon/daemon.cpp 56.27% <100.00%> (+0.16%) ⬆️

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 e55cb8e...09d3362. Read the comment docs.

Copy link
Collaborator

@ricab ricab left a comment

Choose a reason for hiding this comment

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

I am ok with this in general, but I would ask that the check be extracted into a validate_image auxiliary function (receiving the vault and the workflow_provider) and that that function be called from validate_create_arguments, which would need to receive the full config (or even be made a private method of the daemon).

That would allow dropping the comment explaining that the purpose of the code is validating the image (clear from the function name) and avoid adding to an already very long function, where individual calls are easily missed.

Copy link
Collaborator

@ricab ricab left a comment

Choose a reason for hiding this comment

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

Good by me.

bors d=townsend2010

@bors
Copy link
Contributor

bors bot commented Jul 9, 2021

✌️ townsend2010 can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

Copy link
Contributor Author

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

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

One thing inline.

@townsend2010
Copy link
Contributor Author

bors r=ricab

bors bot added a commit that referenced this pull request Jul 9, 2021
2171: [daemon] Fix regression in Workflows-based launches r=ricab a=townsend2010

Fixes #2170

Co-authored-by: Chris Townsend <[email protected]>
Co-authored-by: Ricardo Abreu <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jul 9, 2021

Build failed:

@townsend2010
Copy link
Contributor Author

Sigh...

bors retry

@bors
Copy link
Contributor

bors bot commented Jul 9, 2021

Build failed:

@townsend2010 townsend2010 merged commit 4a4a771 into main Jul 9, 2021
@bors bors bot deleted the fix-workflow-launch-regression branch July 9, 2021 19:29
Saviq pushed a commit that referenced this pull request Jul 12, 2021
2171: [daemon] Fix regression in Workflows-based launches r=ricab a=townsend2010

Fixes #2170

Co-authored-by: Chris Townsend <[email protected]>
Co-authored-by: Ricardo Abreu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[regression] Workflow based launches are broken
2 participants