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

many: add default-configure hook #12701

Merged

Conversation

ernestl
Copy link
Collaborator

@ernestl ernestl commented Apr 4, 2023

Spec: https://docs.google.com/document/d/1QDlVmTCtRAe0SxyvMZ97NlcosEtf1-MCmGcsRixDbek

Two parts:
(1) Implementation and unit tests: #12701 <- this is part 1
(2) Spread test: #12747

snap/hooktypes.go:

  • Added support for default-configure hook type

overlord/configstate/configmgr.go:

  • Register the new default configuration handler

overlord/configstate/configstate.go:

  • Added DefaultConfigure that provides the task set for the default configure hook. It is significantly simplified compared to the configure hook due to simpler mandate to configure snap with gadget defaults on first install for non-core snaps.

overlord/configstate/hooks.go:

  • Created new type defaultConfigureHandler with functions that satisfy the same interface as ConfigureHandler with Before, Error and Done.
  • Modified configure hook Before to only apply defaults if default-configure is not available, otherwise "
    configuration empty change" as per specification.
  • Default Configure hook Before returns with error if there is not a configure hook so that "default-configure will be taken into account only if there is also a configure hook" as per specification.

overlord/snapstate/check_snap.go:

  • New check to flag default-configure without configure

overlord/snapstate/snapstate.go:

  • Added DefaultConfigure after installation and right before service starting task.

@github-actions github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Apr 4, 2023
@ernestl ernestl force-pushed the EL_SNAPDENG-6486_Default_Configure_Hook branch 2 times, most recently from 1b5ee05 to 5e99685 Compare April 4, 2023 17:06
@ernestl ernestl force-pushed the EL_SNAPDENG-6486_Default_Configure_Hook branch from 5e99685 to 0480d37 Compare April 11, 2023 14:58
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

This looks very nice, one question inline and my other wondering is what it needs to go from "draft" to real review status :) ?

@ernestl ernestl force-pushed the EL_SNAPDENG-6486_Default_Configure_Hook branch from 2b7e641 to 3b2cace Compare April 18, 2023 10:59
@ernestl ernestl marked this pull request as ready for review April 18, 2023 11:41
@ernestl ernestl added Needs Samuele review Needs a review from Samuele before it can land Squash-merge Please squash this PR when merging. labels Apr 18, 2023
@ernestl ernestl requested a review from pedronis April 18, 2023 11:44
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.

did a pass, some now wondering about the behavior if the configure hook is missing

@ernestl ernestl requested review from mvo5 and pedronis April 20, 2023 12:43
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.

lgtm, thanks, a comment about build time and validation in general

Copy link
Contributor

@miguelpires miguelpires left a comment

Choose a reason for hiding this comment

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

LGTM, just one question about hook errors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Documentation -auto- Label automatically added which indicates the change needs documentation Needs Samuele review Needs a review from Samuele before it can land Squash-merge Please squash this PR when merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants