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

chore(test): Add config file option to e2e metal tooling #249

Merged
merged 9 commits into from
Nov 12, 2021

Conversation

Callisto13
Copy link
Member

@Callisto13 Callisto13 commented Nov 11, 2021

Note: the last 2 commits are file formatting only and disguise other things which have happened

What this PR does / why we need it:

This PR adds a bit of nice UX to the e2e python tool.

Users can now pass a config file:

./test/tools/run.py -c <path to file>

Which looks like this:

---
org_id: "test-org" # required
project: "test-project" # or project ID
test:
  skip_delete: false # skip the test cleanup and process teardown steps
  skip_teardown: false # skip tearing down the equinix infra
  skip_dmsetup: true # skip the thinpool setup, this should always be true for equinix
  containerd_log_level: "debug"
  flintlock_log_level: "2"
device:
  name: "test-device" # omit if device.id set
  id: "abcdef123456" # set if using existing device
  ssh_key_name: "test-key"
  # the following mirror the equinix options
  # see https://metal.equinix.com/developers/docs/ for values
  userdata: "echo hello"
  plan: "c1.small.x86"
  metro: "sv"
  operating_system: "ubuntu_18_04"
  facility: "ewr1"
  billing_cycle: "hourly"

Each new instance of Config will be initialised with a set of default
config. These are just the values I happened to be hardcoding before,
and there was no special reason for any of them, but they work.

Config can then be loaded from a file and validated against a schema.
This config will then overwrite the appropriate values in the object
config.

Config can also be altered via flags. The 'final' config at that point
can be validated.

The validation is all a bit naive right now as I didn't want to get too
into it.

Not all device config options exist yet, but it is fairly trivial to add as
needed later.

Special notes for your reviewer:

Checklist:

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

@Callisto13 Callisto13 added the kind/feature New feature or request label Nov 11, 2021
@Callisto13 Callisto13 force-pushed the metal-params branch 2 times, most recently from 63e8bf5 to 7f8500d Compare November 11, 2021 16:24
@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2021

Codecov Report

Merging #249 (aa5916e) into main (fe9bb9f) will decrease coverage by 15.52%.
The diff coverage is 41.78%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #249       +/-   ##
===========================================
- Coverage   55.81%   40.29%   -15.53%     
===========================================
  Files          44       46        +2     
  Lines        2012     2169      +157     
===========================================
- Hits         1123      874      -249     
- Misses        781     1236      +455     
+ Partials      108       59       -49     
Impacted Files Coverage Δ
core/application/commands.go 71.66% <ø> (+2.70%) ⬆️
core/application/reconcile.go 0.00% <ø> (ø)
core/models/vmid.go 90.24% <ø> (ø)
core/models/volumes.go 0.00% <ø> (ø)
core/steps/microvm/delete.go 100.00% <ø> (ø)
core/steps/runtime/dir_create.go 73.07% <ø> (ø)
core/steps/runtime/dir_delete.go 81.81% <ø> (ø)
core/steps/runtime/initrd_mount.go 100.00% <ø> (ø)
core/steps/runtime/kernel_mount.go 100.00% <ø> (ø)
infrastructure/containerd/content.go 0.00% <0.00%> (-100.00%) ⬇️
... and 37 more

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 3094d48...aa5916e. Read the comment docs.

@Callisto13 Callisto13 force-pushed the metal-params branch 2 times, most recently from ba5571e to a560674 Compare November 11, 2021 17:07
@Callisto13 Callisto13 marked this pull request as ready for review November 11, 2021 17:07
@Callisto13 Callisto13 force-pushed the metal-params branch 3 times, most recently from 3657b8a to 3cb4762 Compare November 12, 2021 09:46
@Callisto13 Callisto13 requested a review from a team November 12, 2021 09:47
Copy link
Contributor

@yitsushi yitsushi left a comment

Choose a reason for hiding this comment

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

Standard python style is to use multiples of 4 spaces as indentation.
Even if it stays at 3 spaces, the indentation is not consistent, there are places where it's 4 spaces and there are places where it's 3 spaces. All can be auto-fixed with autoformat tools like autopep8, yapf -i or black.

Other than that, some comments, but lgtm

test/tools/config.py Outdated Show resolved Hide resolved
test/tools/config.py Outdated Show resolved Hide resolved
test/tools/config.py Outdated Show resolved Hide resolved
test/tools/config.py Outdated Show resolved Hide resolved
@Callisto13
Copy link
Member Author

Standard python style is to use multiples of 4 spaces as indentation.
Even if it stays at 3 spaces, the indentation is not consistent, there are places where it's 4 spaces and there are places where it's 3 spaces. All can be auto-fixed with autoformat tools like autopep8, yapf -i or black.

lol i thought the spacing looked a bit different between files 😂 , but i didn;t want to do all that before review otherwise it would make the general pr diff huge. i will format them all right before merge

@Callisto13 Callisto13 force-pushed the metal-params branch 5 times, most recently from ce559e5 to fc798f0 Compare November 12, 2021 14:34
A new instance of Config will be initialised with a set of default
config. These are just the values I happened to be hardcoding before,
and there was no special reason for any of them, but they work.

Config can then be loaded from a file and validated against a schema.
This config will then overwrite the appropriate values in the object
config.

Config can also be altered via flags. The 'final' config at that point
can be validated.

The validation is all a bit naive right now as I didn't want to get too
into it.
The first step, simply loads the file and reads the config into the Test
object.

Generation of default flag values is now handled in the default config
setup of the Config class.
Creation of userdata and default name values is handled in the Config
class now.
Also create a handy help script for passing flags to the e2e tests
It was not working because I was using params from the wrong object.
Needs to be on the metal.
@Callisto13 Callisto13 merged commit 046f5eb into liquidmetal-dev:main Nov 12, 2021
@Callisto13 Callisto13 deleted the metal-params branch November 12, 2021 14:54
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants