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 time range to tenants #120

Merged
merged 5 commits into from
Mar 3, 2023
Merged

Conversation

anothertobi
Copy link
Contributor

@anothertobi anothertobi commented Feb 20, 2023

Summary

Introduces a time range for the source/target links of a tenant.

Checklist

  • 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
  • Update tests.

@anothertobi
Copy link
Contributor Author

anothertobi commented Feb 20, 2023

Draft since this does not yet handle existing tenants where the time is outside the time range.

When a tenant is created, the time of the run is used (oldTime). A subsequent run could have a time that is before the previously used lower bound (newTime).

Idea:
Set lower bound of existing tenant to newTime (move during back in time) when source and target match and no other target exists in the time range. If another target for the source exists in the time range between newTime and the upper bound of a source, create a new tenant with the lower bound newTime and an upper bound of the lower bound of the other source (-1h).

@bastjan wdyt?

@anothertobi
Copy link
Contributor Author

related PR: appuio/appuio-io-docs#145

@anothertobi anothertobi force-pushed the feature/time-based-billing-link branch from 49647a2 to 2d1b921 Compare February 23, 2023 17:35
@anothertobi
Copy link
Contributor Author

Decided to fail if there is an overlap in the time range for a tenant instead of covering small edge-cases.

@anothertobi anothertobi marked this pull request as ready for review February 23, 2023 17:39
Copy link
Contributor

@bastjan bastjan left a comment

Choose a reason for hiding this comment

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

The original unique index needs dropping. Otherwise LGTM 👍

pkg/check/missing_test.go Outdated Show resolved Hide resolved
pkg/invoice/invoice_test.go Outdated Show resolved Hide resolved
pkg/report/report.go Show resolved Hide resolved
pkg/db/migrations/0013_tenants_add_timerange.sql Outdated Show resolved Hide resolved
@bastjan
Copy link
Contributor

bastjan commented Feb 24, 2023

Also don't forget the invoice part, from my quick reading it should just work™️ but to feel better i'd add a split timerange to one of the invoice tests https://github.com/appuio/appuio-cloud-reporting/blob/master/pkg/invoice/invoice_test.go.

bastjan added a commit that referenced this pull request Feb 24, 2023
bastjan added a commit that referenced this pull request Feb 24, 2023
…prometheus metric (#121)

* Add `tenantmapping` subcommand to map tenants (source,target) from a prometheus metric
* Update migration name to correspond with #120
pkg/report/report.go Outdated Show resolved Hide resolved
@anothertobi anothertobi force-pushed the feature/time-based-billing-link branch from 4f28b0d to 160a601 Compare March 2, 2023 13:21
@anothertobi anothertobi requested a review from bastjan March 2, 2023 13:21
@bastjan
Copy link
Contributor

bastjan commented Mar 3, 2023

I forgot our discussions already but did we not say we'll create tenants from query timestamp to infinity when upserting them? 🙈

@anothertobi
Copy link
Contributor Author

I forgot our discussions already but did we not say we'll create tenants from query timestamp to infinity when upserting them? 🙈

I think we decided on the default of -infinity, infinity+ or fail because we can't guarantee queries to be run in order.

Copy link
Contributor

@bastjan bastjan left a comment

Choose a reason for hiding this comment

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

Must be rebased, otherwise LGTM

pkg/invoice/invoice_test.go Outdated Show resolved Hide resolved
@anothertobi anothertobi force-pushed the feature/time-based-billing-link branch from 160a601 to 4111e0e Compare March 3, 2023 13:26
@anothertobi anothertobi merged commit 562ae0e into master Mar 3, 2023
@anothertobi anothertobi deleted the feature/time-based-billing-link branch March 3, 2023 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants