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

o/ifacestate: properly undo setup-profiles on component installation #13977

Merged

Conversation

andrewphelpsj
Copy link
Collaborator

@andrewphelpsj andrewphelpsj commented May 17, 2024

This corrects and tests the behavior of undoing a setup-profiles task that contains components. We change setupProfilesForSnap to be setupProfilesForAppSet, so we can pass in a SnapAppSet that doesn't contain the components being set up for the current task when undoing the task.

Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

this seems a strange approach unlike any other undo logic

overlord/snapstate/snapmgr.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

thanks, small comment

return err
}

appSet, err := appSetForTask(task, snapInfo)
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe worth adding a comment here, about what is included in the appset

return err
}

appSet, err := appSetForSnapRevision(st, snapInfo)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

@andrewphelpsj andrewphelpsj force-pushed the fix-component-setup-profiles-undo branch from 6c2cda2 to 89a5fad Compare May 23, 2024 18:13
Copy link
Member

@alfonsosanchezbeato alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

This allows us to pass in a different app sets in doSetupProfiles and in
undoSetupProfiles. The latter will not have components that were just
set up in it.
@andrewphelpsj andrewphelpsj force-pushed the fix-component-setup-profiles-undo branch from 89a5fad to 3766d45 Compare May 29, 2024 17:39
@ernestl ernestl merged commit 2e53b2f into snapcore:master Jun 3, 2024
44 of 49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants