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

refactor: merge create and update plans and update references #192

Merged
merged 1 commit into from
Nov 2, 2021

Conversation

jmickey
Copy link
Contributor

@jmickey jmickey commented Nov 2, 2021

What this PR does / why we need it:

Merges create and update plans into a single plan builder. The update plan currently has no implementation. Update references to the plan.

The update plan code branch is reached if vm.Spec.UpdatedAt property is not set (defaults to 0).

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #156

Checklist:

  • squashed commits into logical changes

@jmickey jmickey added the kind/refactor Only refactoring changes label Nov 2, 2021
@jmickey jmickey requested a review from a team November 2, 2021 09:50
@jmickey jmickey self-assigned this Nov 2, 2021
@github-actions github-actions bot added the size/s label Nov 2, 2021
Comment on lines 5 to 6
MicroVMCreatePlanName = "microvm_create"
MicroVMUpdatePlanName = "microvm_update"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need these two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, removed

Comment on lines +59 to +63
case p.vm.Spec.UpdatedAt != 0:
err = p.update(ctx, ports)
default:
err = p.create(ctx, ports)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this completely and just walk through with all the steps, like "create microvm" and the step will say "no it's running, we don't have to do this".

@richardcase: Any idea/plan/suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had considered this, but I think the mental overhead of keeping them separate isn't very large, and until we actually implement the update logic it's hard to know how different the two are.

@jmickey jmickey force-pushed the refactor/merge-creat-update-plan branch from 886f1e9 to bbff50c Compare November 2, 2021 10:25
@codecov-commenter
Copy link

Codecov Report

Merging #192 (886f1e9) into main (7b95d77) will increase coverage by 0.54%.
The diff coverage is 71.92%.

❗ Current head 886f1e9 differs from pull request most recent head bbff50c. Consider uploading reports for the commit bbff50c to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #192      +/-   ##
==========================================
+ Coverage   54.89%   55.44%   +0.54%     
==========================================
  Files          45       44       -1     
  Lines        1991     1984       -7     
==========================================
+ Hits         1093     1100       +7     
+ Misses        791      777      -14     
  Partials      107      107              
Impacted Files Coverage Δ
core/application/errors.go 0.00% <ø> (ø)
core/application/reconcile.go 0.00% <0.00%> (ø)
core/models/volumes.go 0.00% <ø> (ø)
core/plans/microvm_create_update.go 54.87% <53.33%> (ø)
core/application/query.go 100.00% <100.00%> (ø)
pkg/queue/queue.go 100.00% <0.00%> (+6.06%) ⬆️

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 bd8dac9...bbff50c. Read the comment docs.

@jmickey jmickey merged commit 81434ee into main Nov 2, 2021
@jmickey jmickey deleted the refactor/merge-creat-update-plan branch November 2, 2021 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/refactor Only refactoring changes size/s
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge create and update plans
3 participants