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.

Sign up for GitHub

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

Jump to bottom

Remove go-lxc dependencies #5

Merged
merged 3 commits into from
Oct 25, 2022
Merged

Remove go-lxc dependencies #5

merged 3 commits into from
Oct 25, 2022

Conversation

masnax
Copy link
Contributor

@masnax masnax commented Oct 24, 2022

Reimplements SetupTrust from LXD so we don't import too much from LXD. I think this is a simple enough function that we can duplicate it, not sure if we should bloat LXD's shared package with it.

@masnax masnax requested a review from stgraber October 24, 2022 17:32
tomponline
tomponline reviewed Oct 24, 2022
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

I think this would be fine to add to shared.

tomponline
tomponline requested changes Oct 24, 2022
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment •
edited

Choose a reason for hiding this comment

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

Pr description is a little confusing as go-lxc is the go wrapper of liblxc not lxd's go client :)
Or is this an indirect dependency you're trying to avoid?
Does this also need a go mod update?

@masnax
Copy link
Contributor Author

masnax commented Oct 24, 2022

Pr description is a little confusing as go-lxc is the go wrapper of liblxc not lxd's go client :) Or is this an indirect dependency you're trying to avoid? Does this also need a go mod update?

Yeah I'm trying to avoid bringing in liblxc.

@tomponline
Copy link
Member

Thanks, ive merged the lxd pr now.

@stgraber
Copy link
Contributor

@masnax I take it this needs an update for the LXD change as well as including a gomod update?

@masnax
microcloud/go: Update gomod
a64e35b 
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
@masnax
microcloud/service/lxd/join: Implement SetupTrust instead of importing
e21305e 
Reimplements SetupTrust from LXD so we don't import too much from LXD. I
think this is a simple enough function that we can duplicate it, not
sure if we should bloat LXD's shared package with it.

Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
@masnax
Copy link
Contributor Author

masnax commented Oct 25, 2022

@stgraber I've updated this now for the LXD changes. Also since the LXD changes include the new init types, I've gone ahead and added a commit changing the initialization to use the lxd init --preseed command, as discussed with @tomponline

tomponline
tomponline approved these changes Oct 25, 2022
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Lgtm

@tomponline
Copy link
Member

Good to see all of those indirect dependencies removed.

@masnax
microcloud/service/lxd: Use 'lxd init --preseed' command to init LXD
07c2bc9 
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
@masnax
Copy link
Contributor Author

masnax commented Oct 25, 2022

Just fixed a small typo (The preseed yaml was both the last argument and being written to stdin)

@stgraber stgraber merged commit 09b2246 into canonical:main Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomponline tomponline tomponline approved these changes

@stgraber stgraber Awaiting requested review from stgraber

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@masnax @tomponline @stgraber