Skip to content

Conversation

@everettraven
Copy link
Contributor

@everettraven everettraven commented Jul 19, 2024

Description

  • Updates the RestConfigMapper to use a new TokenTripper that wraps a http.RoundTripper to always set the Authorization header to the referenced ServiceAccount
  • Updates the E2E ServiceAccounts introduced in ✨ Wire up Service Account #1038 to use a scoped set of permissions instead of giving it cluster-admin equivalent permissions (used bind + escalate for RBAC to keep the SA permissions as succinct as possible. Open to changing this if desired)
  • Updates the ClusterExtensionReconciler to have an exported field that can be used to configure the contentmanager.Watcher that should be used to establish watches on the ClusterExtension managed objects
  • Updates the contentmanager.Instance.Watch() method to create watches with the same configurations as existed in the ClusterExtensionReconciler.reconcile() method previously
  • Adds an E2E test to verify that when a change is made to managed objects (in the test case we delete a managed object), that the managed object triggers reconciliation and changes are reverted (in the test case we ensure the managed object is recreated)

resolves #975
resolves #983

Reviewer Checklist

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

@everettraven everettraven requested a review from a team as a code owner July 19, 2024 18:27
@netlify
Copy link

netlify bot commented Jul 19, 2024

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 586046f
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/669e71fa5220dd0008608ebf
😎 Deploy Preview https://deploy-preview-1074--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov
Copy link

codecov bot commented Jul 19, 2024

Codecov Report

Attention: Patch coverage is 65.62500% with 11 lines in your changes missing coverage. Please review.

Project coverage is 72.82%. Comparing base (58c5776) to head (586046f).

Files Patch % Lines
internal/authentication/tripper.go 61.53% 3 Missing and 2 partials ⚠️
internal/authentication/tokengetter.go 0.00% 4 Missing ⚠️
internal/contentmanager/contentmanager.go 83.33% 1 Missing ⚠️
...nternal/controllers/clusterextension_controller.go 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1074      +/-   ##
==========================================
- Coverage   72.93%   72.82%   -0.11%     
==========================================
  Files          31       32       +1     
  Lines        1862     1866       +4     
==========================================
+ Hits         1358     1359       +1     
- Misses        366      368       +2     
- Partials      138      139       +1     
Flag Coverage Δ
e2e 56.80% <65.62%> (+<0.01%) ⬆️
unit 44.31% <9.37%> (-0.10%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@everettraven everettraven force-pushed the feature/wire-sa-cache branch from 941737c to 274df7e Compare July 22, 2024 14:20
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2024
@everettraven everettraven force-pushed the feature/wire-sa-cache branch from 274df7e to b2dc337 Compare July 22, 2024 14:23
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2024
@everettraven everettraven force-pushed the feature/wire-sa-cache branch from b2dc337 to 5e51907 Compare July 22, 2024 14:30
Signed-off-by: everettraven <[email protected]>
Copy link
Contributor

@skattoju skattoju left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci
Copy link

openshift-ci bot commented Jul 22, 2024

@skattoju: changing LGTM is restricted to collaborators

Details

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

acg, err := action.NewWrappedActionClientGetter(cfgGetter,
helmclient.WithFailureRollbacks(false),
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: change in whitespace

},
})).
Build(r)

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: whitespace-only change

Copy link
Contributor

@tmshort tmshort left a comment

Choose a reason for hiding this comment

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

/lgtm
Just small wicked minor nits.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2024
@tmshort tmshort added this pull request to the merge queue Jul 22, 2024
Merged via the queue into operator-framework:main with commit b73ea5c Jul 22, 2024
perdasilva pushed a commit to LalatenduMohanty/operator-controller that referenced this pull request Aug 13, 2024
* wire up serviceaccount based caching layer

Signed-off-by: everettraven <[email protected]>

* remove body close

Signed-off-by: everettraven <[email protected]>

---------

Signed-off-by: everettraven <[email protected]>
perdasilva pushed a commit to kevinrizza/operator-controller that referenced this pull request Aug 13, 2024
* wire up serviceaccount based caching layer

Signed-off-by: everettraven <[email protected]>

* remove body close

Signed-off-by: everettraven <[email protected]>

---------

Signed-off-by: everettraven <[email protected]>
@skattoju skattoju mentioned this pull request Sep 25, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Reconciliation of cluster extension content not working on main branch Wire up dynamic caching layer to manage installed content

4 participants