-
Notifications
You must be signed in to change notification settings - Fork 43
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
Reorganize init configuration #138
Conversation
309a20b
to
b3d7ddc
Compare
In order to fix a disk adding bug (which is also fixed in this refactor), I've created a new PR #139 and rebased this branch off of that one, so that one should now be merged first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this refactor looks much clearer and simpler. That being said, as you are already refactoring, I think now is a good time to add some tests to make sure the changes you have made are working as intended.
@markylaing yeah @masnax discussed a similar concern in our 1:1 this week. |
It would be useful to get a test script, similar to canonical/lxd-ci#3 added for microcloud setup, then, at least for now, it can be run manually to check for regressions. And when/if we get private GH runners that support VMs we can automate it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small changes needed, but overall this looks much clearer and more maintainable, thanks!
One thing I did notice throughout, and something I've wondered about before, is where does microcloud log to? I didn't see much logging (if any at all), and I think it would be useful to log more so we can see where things are failing, what do you think?
Already working on tests :) |
Just to syslog. There is a bit of a dearth of logs though, so I'll try to think about how I can incorporate them some more. |
be42f72
to
cfb950b
Compare
@tomponline If you're OK with it, I think we can go ahead and merge this PR now, since the system tests have been running against the changes introduced here on my system. |
I cannot merge it with the tests failing on compile errors though:
https://github.com/canonical/microcloud/actions/runs/6164591577/job/16730614381?pr=138 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code doesn't current compile
Once its green, can you re-request a review by click the button next to my name as a reviewer so it appears in my todo list here https://github.com/pulls/review-requested Thanks! |
Ah, looks like the LXD cli change is the culprit because the workflow runs I'll need to send out some PRs for LXD Cloud, MicroOVN, and MicroCeph too. |
This consolidates all the commonly used API structs by MicroCloud into a set of helpers that build them and fill in the necessary boilerplate. With multiple forms of creating a MicroCloud cluster (interactive, auto, preseed, add), this can help consolidate a lot of duplicate code. Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
InitSystem will represent all the init configuration collected by the interactive CLI for each peer. A map of InitSystem will be passed around the CLI functions and slowly added to, before it is finally used to initialize the cluster. This will hopefully make debugging MicroCloud much easier as we will have a snapshot of all collected information throughout the init process. It might also facilitate some testing of the CLI. Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
…equests Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
…m configuration Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Static analysis failures |
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
@tomponline Looks to be passing now, so you can merge when ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
And just like that, MicroCloud |
Yay! Thanks I turned off requiring signed commits for now btw |
As that was preventing merging |
In an effort to make it easier to debug the built-up configuration of
microcloud init
andmicrocloud add
, and also easier to validate, this PR reorganizes the init configuration around a map of structs which will hold all the configuration obtained by the init prompts.The main change here is the introduction of the
InitSystem
type which maintains a record of all configuration that is built up over the user prompts. The type holds lists of API structs to be included in the join config or run after cluster setup. The API structs are mainly boilerplate, so some template functions have been introduced to reduce duplication around that as well.This will also help with the upcoming preseed PR which will just need to validate the
yaml
payload into the samemap[string]InitSystem
.