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

fix: check for unsat resolutions in Reconcile #352

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

joelanford
Copy link
Member

Description

Closes #330

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@joelanford joelanford requested a review from a team as a code owner August 22, 2023 15:01
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #352 (088aaa7) into main (fd7cfb5) will decrease coverage by 0.71%.
Report is 3 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #352      +/-   ##
==========================================
- Coverage   82.84%   82.13%   -0.71%     
==========================================
  Files          21       21              
  Lines         892      907      +15     
==========================================
+ Hits          739      745       +6     
- Misses        107      114       +7     
- Partials       46       48       +2     
Flag Coverage Δ
e2e 62.29% <100.00%> (-0.38%) ⬇️
unit 77.22% <6.66%> (-1.38%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
internal/controllers/operator_controller.go 79.92% <100.00%> (-2.51%) ⬇️

Makefile Outdated
@@ -113,7 +113,7 @@ test-unit: $(SETUP_ENVTEST) ## Run the unit tests
e2e: KIND_CLUSTER_NAME=operator-controller-e2e
e2e: KUSTOMIZE_BUILD_DIR=config/e2e
e2e: GO_BUILD_FLAGS=-cover
e2e: run kind-load-test-artifacts test-e2e e2e-coverage kind-cluster-cleanup ## Run e2e test suite on local kind cluster
e2e: kind-cluster-cleanup run kind-load-test-artifacts test-e2e e2e-coverage kind-cluster-cleanup ## Run e2e test suite on local kind cluster
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a preemptive cleanup of a pre-existing e2e cluster. I was running into the slight annoyance of the e2e failing, the last kind-cluster-cleanup not running, and then having to manually delete the cluster to be able to run the e2e again.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see @tmshort has a more wide-ranging fix in #353, so removing the Makefile changes from this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding kind-cluster-cleanup early in the dependency line won't work; as make will have recognized that target as already completed by the time it gets to the second kind-cluster-cleanup. So it won't be run, and it will never clean up!

Comment on lines +143 to +147
// TODO: Checking for unsat is awkward using the current version of deppy.
// This awkwardness has been fixed in an unreleased version of deppy.
// When there is a new minor release of deppy, we can revisit this and
// simplify this to a normal error check.
// See https://github.com/operator-framework/deppy/issues/139.
Copy link
Member Author

Choose a reason for hiding this comment

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

On the working group call today, I mentioned that I would try to go ahead and bump deppy to the commit containing this fix.

Unfortunately that commit brings in some breaking changes with deppy's removal of the EntitySource API, so we'll have to wait until operator-controller accounts for that change. (see #347)

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
@m1kola m1kola added this pull request to the merge queue Aug 23, 2023
Merged via the queue into operator-framework:main with commit b99cc20 Aug 23, 2023
10 of 11 checks passed
@joelanford joelanford deleted the fix-unsat-check branch August 23, 2023 12:56
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.

Reconciler does not correctly handle UNSAT resolutions
4 participants