Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

Move timezone and utils to separate package #2015

Merged
merged 2 commits into from
Jul 21, 2022

Conversation

pub-djedruszczak
Copy link
Contributor

This PR moves the api.timeZone var and certain utilities from the api package into a new tz package. The GetFromTo function is exported for use in a future native implementation of the linearRegression function.

Some encapsulation of the api.timeZone var (now tz.TimeZone) is lost so that it can continue to be set from api.ConfigSetup, maintaining backwards compatibility. The api package is highly dependent on the expr package, making the export of api.getFromTo from the api package for use in native function implementations (the expr package) difficult due to import cycles.

Copy link
Contributor

@Dieterbe Dieterbe left a comment

Choose a reason for hiding this comment

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

in master, the timezone is considered an api level configuration, as various of the api methods - before involving expr - call getFromTo, and thus need access to the default timezone configured.
I'm not sure why linearRegression would need to access that variable. Is it because from/to can be any 'at' style expression, which allows specifying a timezone? wouldn't that timezone then be scoped to that function call only? it has no bearing on the request-scoped from/to, correct? do you envision needing only read access or also writing to the timezone variable?

also, while i never particularly liked the idea of bringing all api models together in one 'models' package, that is how it is, and moving just a single model ( FromTo ) elsewhere is definitely counter to our current code structure.

@stale
Copy link

stale bot commented May 3, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 3, 2022
@robert-milan
Copy link
Contributor

I'll be taking over reviewing this for now

@stale stale bot removed the stale label Jun 10, 2022
@CLAassistant
Copy link

CLAassistant commented Jun 15, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@robert-milan robert-milan left a comment

Choose a reason for hiding this comment

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

Yes, we are slightly departing from our traditional layout of Metrictank files / models / separation of code, however I think we should just move forward with this. If it becomes a problem in the future we can always change it.

@robert-milan
Copy link
Contributor

Can we get @djedruszczak to agree to the CLA?

@pub-djedruszczak
Copy link
Contributor Author

Can we get @djedruszczak to agree to the CLA?

Signed!

@robert-milan robert-milan merged commit 6534a8f into grafana:master Jul 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants