-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Copy cortex/pkg/querier/astmapper
package dependency into Loki
#4982
Conversation
- Use _ in argument of method `pkg/querier/astmapper/subtree_folder.go` since it is not being used - Remove unnecessary cast from `subtree_folder.go`
There was a problem hiding this 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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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
CHANGELOG.md
about the changes.