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

fix: handle ill-formed profiles created by older rockcraft #588

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

cmatsuoka
Copy link
Collaborator

Older versions of rockcraft may have created a rockcraft lxd project
containing a broken default profile. If this is the case, tell
the user to delete the project and start over.

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox?

Check if the lxd snap is installed by querying the snapd socket
instead of looking for an executable in the path, as lxd can use
auto-installer stubs.

Co-authored-by: Callahan Kovacs <callahan.kovacs@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka cmatsuoka requested review from tigarmo and mr-cal June 24, 2024 20:05
@cmatsuoka cmatsuoka marked this pull request as draft June 24, 2024 20:13
Older versions of rockcraft may have created a rockcraft lxd project
containing a broken default profile. If this is the case, tell the
user to delete the project and start over.

Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
if not devices:
# Project exists but the default profile is ill-formed, tell the user to
# delete the project and start over.
raise LXDError(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately deleting the broken projects ourselves involves deleting everything the project is using first, and this is too risky to implement without better investigation and careful testing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this you be a good scenario to have a test for, aside from that, I generally approve of the appoach

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this new check should have a test

@cmatsuoka cmatsuoka marked this pull request as ready for review June 24, 2024 20:38
Copy link
Collaborator

@mr-cal mr-cal left a comment

Choose a reason for hiding this comment

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

Is this a duplicate of #436?

Copy link
Contributor

@tigarmo tigarmo left a comment

Choose a reason for hiding this comment

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

I believe there are failing unit tests

@cmatsuoka
Copy link
Collaborator Author

Is this a duplicate of #436?

It's a similar check, but performed on the specific project's default profile instead of the default project default profile.

@tigarmo tigarmo force-pushed the rockcraft branch 2 times, most recently from 27b62e2 to 18ca5f3 Compare July 2, 2024 13:34
Base automatically changed from rockcraft to main July 3, 2024 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants