Skip to content

Correct state when uninstalling and (re-)installing bundles#2354

Merged
VinozzZ merged 3 commits intogetporter:release/v1from
jarnfast:bugfix/install-uninstall-repeat-ad-nauseum
Sep 14, 2022
Merged

Correct state when uninstalling and (re-)installing bundles#2354
VinozzZ merged 3 commits intogetporter:release/v1from
jarnfast:bugfix/install-uninstall-repeat-ad-nauseum

Conversation

@jarnfast
Copy link
Contributor

What does this change

This changes how the bundle state is calculated in order to support scenarios where a bundle is uninstalled and then installed again. It will handle state calculations when both installed and uninstalled times are set.

Problem originally observed when migrating from v0.38 to v1 causing bundle to be in an uninstalled state

What issue does it fix

Partial fix to #2353

Notes for the reviewer

I'm still not entirely convinced this is the best approach to fix the problem.

An alternative would be to change the behavior of ApplyStatus to ensure that Installed and Uninstalled timestamps cannot be set at the same time. Ie. the bundle is either installed or uninstalled - but never both.

E.g. in storage/installation.go:

// ApplyResult updates cached status data on the installation from the
// last bundle run.
func (i *Installation) ApplyResult(run Run, result Result) {
	// Update the installation with the last modifying action
	// .. <snipped>

	if !i.IsInstalled() && run.Action == cnab.ActionInstall && result.Status == cnab.StatusSucceeded {
		i.Status.Installed = &result.Created
		i.Status.Uninstalled = nil // ensure Uninstalled is nil
	}

	if !i.IsUninstalled() && run.Action == cnab.ActionUninstall && result.Status == cnab.StatusSucceeded {
		i.Status.Uninstalled = &result.Created
		i.Status.Installed = nil // ensure Installed is nil
	}
}

Checklist

  • Did you write tests?
  • Did you write documentation?
  • Did you change porter.yaml or a storage document record? Update the corresponding schema file.
  • If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

  • Comment with /azp run test-porter-release if a magefile or build script was modified
  • Comment with /azp run porter-integration if it's a non-trivial PR

…re set

Signed-off-by: Jens Arnfast <jens@arnfast.net>
Signed-off-by: Jens Arnfast <jens@arnfast.net>
@VinozzZ
Copy link
Contributor

VinozzZ commented Sep 13, 2022

Thank you for submitting the PR. I agree that this is not the optimal solution for us to completely address the issue in terms of how porter handles reinstallation. I have opened another issue # 2356 to specifically track this gap in our design. We will need to do more validation to make sure this use case is properly handled

Signed-off-by: Jens Arnfast <jens@arnfast.net>
Copy link
Contributor

@VinozzZ VinozzZ left a comment

Choose a reason for hiding this comment

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

lgtm, thank you for working through it with us!

@VinozzZ VinozzZ merged commit 1c68206 into getporter:release/v1 Sep 14, 2022
@jarnfast jarnfast deleted the bugfix/install-uninstall-repeat-ad-nauseum branch September 14, 2022 20:49
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.

2 participants