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

Add Builder UpToDate Condition #1370

Merged
merged 3 commits into from
Nov 17, 2023
Merged

Add Builder UpToDate Condition #1370

merged 3 commits into from
Nov 17, 2023

Conversation

tomkennedy513
Copy link
Collaborator

@tomkennedy513 tomkennedy513 commented Nov 11, 2023

Adds a condition for Builder UpToDate. This tracks the status of the latest reconcile of the builder, while the Ready condition indicates that the builder is able to be used in a build. The reason behind this change is that a builder reconcile may fail for a variety of reasons, but as long as there is a latestImage, builds should be able to continue.

resolves #1365

@tomkennedy513 tomkennedy513 changed the title Builder up to date Add Builder UpToDate Condition Nov 11, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2023

Codecov Report

Merging #1370 (1a042b8) into main (7ba039c) will decrease coverage by 0.05%.
Report is 15 commits behind head on main.
The diff coverage is 57.77%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main    #1370      +/-   ##
==========================================
- Coverage   67.29%   67.24%   -0.05%     
==========================================
  Files         133      133              
  Lines        8224     8267      +43     
==========================================
+ Hits         5534     5559      +25     
- Misses       2237     2255      +18     
  Partials      453      453              
Files Coverage Δ
pkg/apis/build/v1alpha2/builder_types.go 22.22% <ø> (ø)
pkg/apis/build/v1alpha2/image_lifecycle.go 0.00% <ø> (ø)
pkg/apis/build/v1alpha2/image_types.go 50.00% <ø> (ø)
pkg/duckbuilder/duck_builder.go 55.55% <100.00%> (+5.55%) ⬆️
pkg/reconciler/image/reconcile_build.go 85.45% <100.00%> (+1.78%) ⬆️
pkg/apis/build/v1alpha2/builder_lifecycle.go 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

tomkennedy513 and others added 3 commits November 13, 2023 11:56
- ConditionReady indicates that the builder can be used in a build (has a latestImage)
- ConditionUpToDate indicates the status of the builder reconciler

Signed-off-by: Tom Kennedy <ktom@vmware.com>
Signed-off-by: Tom Kennedy <ktom@vmware.com>
Signed-off-by: Tom Kennedy <ktom@vmware.com>
@tomkennedy513 tomkennedy513 marked this pull request as ready for review November 13, 2023 17:04
@tomkennedy513 tomkennedy513 requested a review from a team as a code owner November 13, 2023 17:04
@chenbh
Copy link
Contributor

chenbh commented Nov 16, 2023

Feel free to push back if you think this is scope creep, but what do you think about printing a warning in the build logs if the builder is not up to date?

I'm a little bit worried about how to surface this info to the user, I highly doubt people are in the habit of checking their builders/stacks/buildpacks if their Images are happy. While they probably won't check build logs that much either, it would at be something. Maybe we can also bubble this up in the Image as well, but that might be too complex to introduce a "Warning" state in addition to the current "Error" states.

@tomkennedy513
Copy link
Collaborator Author

Feel free to push back if you think this is scope creep, but what do you think about printing a warning in the build logs if the builder is not up to date?

I'm a little bit worried about how to surface this info to the user, I highly doubt people are in the habit of checking their builders/stacks/buildpacks if their Images are happy. While they probably won't check build logs that much either, it would at be something. Maybe we can also bubble this up in the Image as well, but that might be too complex to introduce a "Warning" state in addition to the current "Error" states.

I thought about this but the build doesn't really know anything about the builder resource so I wasn't sure if it made a ton of sense. I'm still open to it but I think I would prefer it in a follow up pr

@chenbh
Copy link
Contributor

chenbh commented Nov 17, 2023

I thought about this but the build doesn't really know anything about the builder resource so I wasn't sure if it made a ton of sense.

I was imagining something simple like a --warn-builder-not-up-to-date=clusterbuilder/my-builder flag in cmd/build-init. So the build reconciler would decide to inject this flag and the prepare step would just dutifully print it.

I'm still open to it but I think I would prefer it in a follow up pr

👍

@tomkennedy513 tomkennedy513 merged commit 6f588dd into main Nov 17, 2023
3 checks passed
@tomkennedy513 tomkennedy513 deleted the builder-up-to-date branch November 17, 2023 16:51
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.

Allow builds to be created if the builder has a latest image
3 participants