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

Cache human readable billing entity name in Organization #90

Merged
merged 3 commits into from
Jan 13, 2023

Conversation

bastjan
Copy link
Contributor

@bastjan bastjan commented Jan 12, 2023

  • Implement /status subresource handling for Organizations
  • Add controller writing this status

Checklist

  • PR contains a single logical change (to build a better changelog).
  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog.

@bastjan bastjan marked this pull request as ready for review January 12, 2023 16:01
@bastjan bastjan self-assigned this Jan 12, 2023
@bastjan bastjan added the enhancement New feature or request label Jan 12, 2023
@bastjan bastjan force-pushed the cache-billingentity-name branch from 21c4624 to c738ad0 Compare January 12, 2023 16:05
- Implement /status subresource handling for Organizations
- Add controller writing this status
@bastjan bastjan force-pushed the cache-billingentity-name branch from c738ad0 to 9db7b90 Compare January 12, 2023 16:06
@bastjan bastjan requested review from glrf and simu January 12, 2023 16:06
Copy link
Contributor

@glrf glrf left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

var be billingv1.BillingEntity
err := r.Client.Get(ctx, client.ObjectKey{Name: org.Spec.BillingEntityRef}, &be)
if err != nil {
return ctrl.Result{}, err
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: I'm not sure if we want to log if the error is not found.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not found is an important error IMO since we validate that the entity exists on linking them.

Copy link
Contributor

@glrf glrf Jan 13, 2023

Choose a reason for hiding this comment

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

Hmm, maybe you're right. I'm not sure how helpful it really is if we log not found repleatedly if someone deletes the BE in Odoo, but you're right it's a relevant error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The good thing about returning an error is you get automated backoff.

Copy link
Member

@simu simu left a comment

Choose a reason for hiding this comment

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

LGTM overall

controllers/org_billingentity_name_cache_controller.go Outdated Show resolved Hide resolved
@bastjan bastjan merged commit 3c7e4c5 into master Jan 13, 2023
@bastjan bastjan deleted the cache-billingentity-name branch January 13, 2023 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants