Skip to content

Backport PreInit hook support, and deprecate PreBootstrap#290

Merged
masnax merged 8 commits into
canonical:v2from
masnax:backport-pre-init
Dec 3, 2024
Merged

Backport PreInit hook support, and deprecate PreBootstrap#290
masnax merged 8 commits into
canonical:v2from
masnax:backport-pre-init

Conversation

@masnax
Copy link
Copy Markdown
Contributor

@masnax masnax commented Nov 14, 2024

k8s-snap has requested PreInit to be included in the LTS as they are dependent on it for their LTS.

This feature clashes with the PreBootstrap hook and was meant to replace it, so instead PreBootstrap is set as deprecated, and will be replaced with a noop function if PreInit is defined by the upstream package.

@masnax
Copy link
Copy Markdown
Contributor Author

masnax commented Nov 14, 2024

While setting it as deprecated is more correct, I'm a bit concerned with breaking pipelines due to linter issues which will need to be ignored or addressed if they update to v2.0.6.

roosterfish
roosterfish previously approved these changes Nov 15, 2024
Copy link
Copy Markdown
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

Looks good to me but I think we should rather release this in a v2.x.0 instead of a patch version as it deprecates existing features.
Whilst thinking more about this probably adding the websocket client function (as an example) would have also been a good candidate for a v2.x.0 instead of v2.0.x.

Either way I think it's not considered to be a breaking change if downstream pipelines will now produce a warning due to the deprecation of the func.
I would even say that is wanted so that they can either switch the func or proactively add an ignore statement themselves.

Copy link
Copy Markdown
Contributor

@bschimke95 bschimke95 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 for the backport!

…init

Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
(cherry picked from commit 6375e4d)
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
(cherry picked from commit cd15984)
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
(cherry picked from commit 5b11527)
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
(cherry picked from commit 675125d)
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
(cherry picked from commit 4201bfa)
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
(cherry picked from commit 21427d3)
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
Signed-off-by: Max Asnaashari <max.asnaashari@canonical.com>
@masnax
Copy link
Copy Markdown
Contributor Author

masnax commented Nov 18, 2024

I've changed this to now error out if both PreBootstrap and PreInit are defined, rather than silently unset PreBootstrap.

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.

3 participants