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

Copy cortex/pkg/querier/astmapper package dependency into Loki #4982

Merged
merged 3 commits into from
Jan 3, 2022

Conversation

DylanGuedes
Copy link
Contributor

What this PR does / why we need it:
As a part of removing Loki dependency on cortex packages, we decided to copy the querier/astmapper package from cortex to Loki itself.
I decided to only fork this package and not whole querier because the other subpackages require code changes and the gigantique diff will make reviews a hard task. For this one I just had to copy stuff and replace cortex/astmapper with the new counterpart.

Which issue(s) this PR fixes:
NA

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated
  • Add an entry in the CHANGELOG.md about the changes.

@DylanGuedes DylanGuedes requested a review from a team as a code owner December 20, 2021 19:01
- Use _ in argument of method `pkg/querier/astmapper/subtree_folder.go`
  since it is not being used
- Remove unnecessary cast from `subtree_folder.go`
Copy link
Collaborator

@trevorwhitney trevorwhitney left a comment

Choose a reason for hiding this comment

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

LGTM. One quesiton about the util_logs package, are we waiting to port those all off cortex at the same time, because I noticed one came over with the astmapper package.

"github.com/go-kit/log/level"
"github.com/prometheus/prometheus/promql/parser"

util_log "github.com/cortexproject/cortex/pkg/util/log"
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this still be pointing to cortex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, until this PR gets merged.

@DylanGuedes
Copy link
Contributor Author

LGTM. One quesiton about the util_logs package, are we waiting to port those all off cortex at the same time, because I noticed one came over with the astmapper package.

Hmm that's a good question. There's already an open PR that get rid of Cortex util/util_logs, but since it is quite big and I'm expecting that it will take some time to get merged, I decided to send this one for review as it might be easier to review/merge.
That said, I'm fine with waiting for the util fork pkg to be merged before this one but I'm also fine with merging this one first..

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

@cyriltovena cyriltovena merged commit d5783e8 into grafana:main Jan 3, 2022
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.

3 participants