Fix inconsistent rebuild #2719
Merged
Fix inconsistent rebuild #2719
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When we publish a bundle, we may need to rebuild it first. Now that build supports --insecure-registry, we should pass that flag from publish to the build command so that build also respects that setting. This was causing the smoke test for the airgap tests to intermittently fail because of _another_ bug, which caused porter to occasionally consider a bundle out-of-date and require rebuilding when in fact nothing had changed. This first commit fixes passing the flag properly from publish to build so that when a bundle is out-of-date and it's pointed at an insecure registry, the build won't fail with https/tls errors. The next commit will fix porter triggering a rebuild at the wrong times. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
When we consider if a previously built bundle is out-of-date and should be rebuilt, we look at the porter.yaml file and also the set of mixins used by the bundle. We were storing those mixins and their installed version in a _map_, which doesn't sort consistently. So when a bundle used more than one mixin, there was always a chance that it would sort differently from the last build and trigger a rebuild. I have updated how we compare the mixins used by the old/new bundle so that we always sort them in alphabetical order. I have also added a bunch of debug logging and trace data so that it's easier to troubleshoot a bundle being incorrectly flagged as out-of-date next time. 😅 Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
carolynvs
force-pushed
the
fix-inconsistent-rebuild
branch
from
63f073c
to
c59a360
Compare
sgettys
reviewed
Apr 11, 2023
pkg/cnab/config-adapter/stamp.go
Outdated
| } | ||
|
|
||
| func (m MixinRecords) Less(i, j int) bool { | ||
| return m[i].Name < m[j].Name |
There was a problem hiding this comment.
Should this be a combination of name and version?
There was a problem hiding this comment.
A bundle can only use a single mixin version at a time, so there can't be exec@1.2.3 and exec@1.2.4 for example, so just sorting by name will work. But who knows what the future holds! I can add version into the mix just in case we change things up later once mixins can be referenced bundles instead of embedded binaries.
sgettys
reviewed
Apr 11, 2023
pkg/cnab/config-adapter/stamp.go
Outdated
| // getUsedMixinsVersion compare the mixins defined in the manifest and the ones installed and then retrieve the mixin's version info | ||
| func (c *ManifestConverter) getUsedMixinsVersion() map[string]string { | ||
| usedMixinsVersion := make(map[string]string) | ||
| // getUsedMixinRecords compare the mixins defined in the manifest and the ones installed and then retrieve the mixin's version info |
There was a problem hiding this comment.
nit: Update comment now that we're returning the record instead of just version info
When sorting mixins for the bundle stamp, sort by the mixin name and also include the mixin version. Currently bundles can only have a single version of a mixin, so the version comparison is for the future, after we have mixins as bundles, where it could be possible perhaps to use two different versions of a mixin in the same bundle, because they will be referenced images instead of embedded binaries. Make sure to sort the versions by semantic version and not string, so that the leading v prefix is ignored and stuff like pre-releases and 0.0.10 and 0.0.1 sort correctly. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
carolynvs
force-pushed
the
fix-inconsistent-rebuild
branch
from
41cc316
to
d749dde
Compare
|
@sgettys I've updated the comparison to take into account name and version to future proof this for when we have mixins as bundles. Please take another look. |
sgettys
approved these changes
Apr 12, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this change
This patch corrects two interrelated issues that were causing our bundle out-of-date check to not work well:
I have updated the calls to EnsureBundleIsUpToDate so that it correctly passes through the --insecure-registry flag. I have also fixed the mixin comparison to use a sorted array so that it consistently generates the same manifest digest when detecting changes to the bundle.
I have also added a bunch of debug messages to the comparison of the cached and current bundle so that when it's flagged as out-of-date, it will print out why and include trace data to help figure out what's wrong/different.
What issue does it fix
Closes #2718
Closes #2503
Closes #500
Notes for the reviewer
N/A
Checklist
Reviewer Checklist