Uh oh!
There was an error while loading. Please reload this page.
🌱 Add feature-gate kustomize files, docs, and demo for webhook support feature - #1996
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
c5a968d to
0dd796bCompareCodecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #1996 +/- ##
=======================================
Coverage 69.11% 69.11% =======================================
Files 79 79 Lines 7023 7023 =======================================
Hits 4854 4854 Misses 1887 1887 Partials 282 282
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Uh oh!
There was an error while loading. Please reload this page.
| @@ -0,0 +1,60 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
However, could we add a GitHub action to run all demos that we have been adding, so that we can avoid breaking any of them? It is nice because it seems like we end up covering the feature to prevent regressions.
There was a problem hiding this comment.
I worry about doing this for every PR as the number of demos will grow and this might get time consuming. I'm also not entirely convinced that breaking a demo should block a PR from merging to main. We may not want to release, but we shouldn't block progress because of something that can be solved separately, imo. So, maybe this is something we might want to bring up in the community meeting for discussion rather than solving in this PR?
There was a problem hiding this comment.
IHMO: If we broke the demo it is like an e2e test,
That means that something that was working stop to work
So, we need to start testing out.
But I either agree that it might be out of the scope of this one
We can create a follow up PR for that and discuss it 👍
There was a problem hiding this comment.
For me, this is more of a "documentation is out of date" issue than a "we're not doing what we are supposed to do" issue. So, I'd be against adding this check on a PR basis:
- e2es should already catch any regressions in behavior (the most important thing imo)
- (as with e2es and unit tests) these will tend to grow in runtime with the number of features/demos added. I think it's worthwhile to have a PR pipeline that is as quick as possible.
- I think it's unlikely that they will yield that many true positives because the failures will likely be around API surface changes (which should change infrequently, at least, after release). If we have false positives (issues with the demo script, dependencies, etc.) it's another set of problems that aren't related to the PR that can increase the lead time to merge.
So, I think from a cost/value perspective, it would be better to put this check on the critical path of the release job and run it once at the end, or have it as a periodic running once every week or two.
There was a problem hiding this comment.
Thanks for your comment – you made some good points! 😊
Just to share another view:
- We tested one today and it worked fine, no flakes.
- These tests check real features, like E2E, If something breaks, it’s probably the feature, not the docs.
(However, +1 for a possible duplication since we can end up have e2e tests and demos doing the same) - Performance might be a problem later, but we can wait and see.
I think your opinion makes sense too, and it’s probably not something we need to fix now.
All good from my side!
camilamacedo86
left a comment
There was a problem hiding this comment.
I have just two small suggestions: 😊
Please update the title to better reflect the changes:
👉 https://github.com/operator-framework/operator-controller/pull/1996/files#r2111555232Could you add a GitHub Action to test the demos?
This will help make sure they always keep working:
👉 https://github.com/operator-framework/operator-controller/pull/1996/files#r2111563033
Everything else looks great to me! 🚀
It would be super helpful to get a review from @tmshort to double-check that the Kustomize changes are safe and won’t cause issues downstream.
Also, tagging @joelanford since he has a clear vision for how the Kustomize files should be organised: https://github.com/operator-framework/operator-controller/pull/1949/files
Thanks a lot! 🙌
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
camilamacedo86
left a comment
There was a problem hiding this comment.
Hey @perdasilva 👋
I had another look at this one:
- We’re just adding the kustomize files, so in theory, it shouldn’t break anything downstream 👍
- Regarding the re-organisation in #1949 – I don’t think we should block this PR because of it. That effort might still evolve, and we can adjust later if needed.
So for me, everything looks good – LGTM 🚀
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -0,0 +1,62 @@ | |||
| ## Installation of Bundles containing Webhooks | |||
There was a problem hiding this comment.
Hey @michaelryanpeter, just for your awareness — since we're adding more +1 drafts here. No pressure at all, just wanted to let you know it's there in case you have bandwidth later. We can always do follow-ups to improve it.
Description
Adds docs and demo for webhook support feature
Reviewer Checklist