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: move thinpool setup to general test Setup/Teardown #223

Merged
merged 1 commit into from
Nov 9, 2021

Conversation

Callisto13
Copy link
Member

@Callisto13 Callisto13 commented Nov 8, 2021

I had this being done in bash via a Docker Entrypoint + trap
combination, but it was quite brittle, and sometimes the teardown would
not run which then meant you had to run a manual step to remove thinpools
and loop devices which was annoying.

This changes brings that into the test so the setup and teardown can be
handled programatically.

There is still a case where some manual cleanup may be required: when the tests
fail and devices cannot be removed. I have a thing on my list to have an aggressive
cleanup of all mvms etc on failure. Coming soon!

There is also a case where some "child" pool devices are not cleaned up. I have deliberately
not done anything about this as we should actually fix the code that is leaving those lying
around after a failed delete.

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2021

Codecov Report

Merging #223 (22cd4d0) into main (fe9bb9f) will increase coverage by 0.45%.
The diff coverage is 70.83%.

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

@@            Coverage Diff             @@
##             main     #223      +/-   ##
==========================================
+ Coverage   55.81%   56.27%   +0.45%     
==========================================
  Files          44       44              
  Lines        2012     1985      -27     
==========================================
- Hits         1123     1117       -6     
+ Misses        781      767      -14     
+ Partials      108      101       -7     
Impacted Files Coverage Δ
core/application/commands.go 71.66% <ø> (+2.70%) ⬆️
pkg/validation/validate.go 100.00% <ø> (+12.50%) ⬆️
core/plans/microvm_create_update.go 57.53% <36.36%> (+2.65%) ⬆️
core/plans/microvm_delete.go 58.49% <100.00%> (+1.62%) ⬆️
core/steps/microvm/create.go 100.00% <100.00%> (ø)
core/steps/microvm/start.go 100.00% <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 ac00c58...dd55e24. Read the comment docs.

@github-actions github-actions bot added size/m and removed size/s labels Nov 8, 2021
@Callisto13 Callisto13 requested a review from a team November 8, 2021 12:36
@Callisto13 Callisto13 changed the title (refactor): move thinpool setup to general test Setup/Teardown refactor: move thinpool setup to general test Setup/Teardown Nov 8, 2021
richardcase
richardcase previously approved these changes Nov 9, 2021
I had this being done in bash via a Docker Entrypoint + `trap`
combination, but it was quite brittle, and sometimes the teardown would
not run which then meant you had to run a manual step.

This changes brings that into the test so the setup and teardown can be
handled programatically.
@Callisto13
Copy link
Member Author

ffs sorry @richardcase can i get a re-click?

gexec.CleanupBuildArtifacts()
}

func makeDirectories() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can add this to a pkg or somewhere, and reuse in cmd/dev-helper and the runner, we could manage this setup from one place.

Copy link
Member Author

Choose a reason for hiding this comment

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

gonna delete cmd/dev-helper instead 😈

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be still better to manage those settings in one place, I like the Go version and we can use this as a replacement for the shell script (and update the documentation to use that instead)

Copy link
Member Author

@Callisto13 Callisto13 Nov 9, 2021

Choose a reason for hiding this comment

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

oh like you mean instead of all the copy-pastable commands we have in the quick-start docs?

for sure, would be good to have a (up to date) "setup all my stuff" command for people.

can i do that later tho :p

@Callisto13 Callisto13 merged commit d60a54e into main Nov 9, 2021
@Callisto13 Callisto13 deleted the thin-setup branch November 9, 2021 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request size/m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants