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

Revert "[loki-canary] Support passing loki address as environment variable (#8024)" #8183

Merged

Conversation

JordanRushing
Copy link
Contributor

What this PR does / why we need it:
This PR reverts commit f9ed9cb which introduced breaking config changes for the Loki canary. The relevant change introduced support for configuring a new LOKI_ADDRESS canary environment variable by adopting a new library for flags.

This is the easiest path forward to avoid forcing breaking config changes on canary operators; such a change is likely better suited to join other configuration changes in a future major Loki release.

Which issue(s) this PR fixes:
N/A

Special notes for your reviewer:
Related PR:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@JordanRushing JordanRushing self-assigned this Jan 17, 2023
@JordanRushing JordanRushing requested a review from a team as a code owner January 17, 2023 20:37
@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Jan 17, 2023
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

1 similar comment
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

Copy link
Collaborator

@slim-bean slim-bean left a comment

Choose a reason for hiding this comment

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

LGTM

@slim-bean
Copy link
Collaborator

@jijotj sorry to have revert this 😬 but unfortunately for all of us the Go stdlib flag parser allows for single dash var names even against long variables.

kingpin will fail to parse all of these as it expects a single dash to be a short name for a config.

Unfortunately this means basically everyones canary install is broken by this change 😞

I think if we were to make a change as big as this we'd want to tie it to a major release.

Since your original PR you were only looking to configure a few vars from the environment, I wonder if a smaller PR to just set those manually could be a compromise here?

@owen-d owen-d merged commit d8a0c6f into grafana:main Jan 17, 2023
JordanRushing added a commit to JordanRushing/loki that referenced this pull request Jan 17, 2023
…iable (grafana#8024)" (grafana#8183)

**What this PR does / why we need it**:
This PR reverts commit f9ed9cb which
introduced breaking config changes for the Loki canary. The relevant
change introduced support for configuring a new `LOKI_ADDRESS` canary
environment variable by adopting a new library for flags.

This is the easiest path forward to avoid forcing breaking config
changes on canary operators; such a change is likely better suited to
join other configuration changes in a future major Loki release.

**Which issue(s) this PR fixes**:
N/A

**Special notes for your reviewer**:
Related PR:
- grafana#8024

**Checklist**
- [x] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [ ] Documentation added
- [ ] Tests updated
- [x] `CHANGELOG.md` updated
- [ ] Changes that require user attention or interaction to upgrade are
documented in `docs/sources/upgrading/_index.md`
jeschkies pushed a commit to jeschkies/loki that referenced this pull request Jan 18, 2023
…iable (grafana#8024)" (grafana#8183)

**What this PR does / why we need it**:
This PR reverts commit f9ed9cb which
introduced breaking config changes for the Loki canary. The relevant
change introduced support for configuring a new `LOKI_ADDRESS` canary
environment variable by adopting a new library for flags.

This is the easiest path forward to avoid forcing breaking config
changes on canary operators; such a change is likely better suited to
join other configuration changes in a future major Loki release.

**Which issue(s) this PR fixes**:
N/A

**Special notes for your reviewer**:
Related PR:
- grafana#8024

**Checklist**
- [x] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [ ] Documentation added
- [ ] Tests updated
- [x] `CHANGELOG.md` updated
- [ ] Changes that require user attention or interaction to upgrade are
documented in `docs/sources/upgrading/_index.md`
JordanRushing added a commit that referenced this pull request Jan 18, 2023
…iable (#8024)" (#8183) (#8184)

This PR cherry-picks d8a0c6f onto k133.
Original description below:

**What this PR does / why we need it**:
This PR reverts commit f9ed9cb which
introduced breaking config changes for the Loki canary. The relevant
change introduced support for configuring a new `LOKI_ADDRESS` canary
environment variable by adopting a new library for flags.

This is the easiest path forward to avoid forcing breaking config
changes on canary operators; such a change is likely better suited to
join other configuration changes in a future major Loki release.

**Which issue(s) this PR fixes**:
N/A

**Special notes for your reviewer**:
Related PR:
- #8024

**Checklist**
- [x] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [ ] Documentation added
- [ ] Tests updated
- [x] `CHANGELOG.md` updated
- [ ] Changes that require user attention or interaction to upgrade are
documented in `docs/sources/upgrading/_index.md`

**What this PR does / why we need it**:

**Which issue(s) this PR fixes**:
Fixes #<issue number>

**Special notes for your reviewer**:

**Checklist**
- [ ] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [ ] Documentation added
- [ ] Tests updated
- [ ] `CHANGELOG.md` updated
- [ ] Changes that require user attention or interaction to upgrade are
documented in `docs/sources/upgrading/_index.md`
@jijotj
Copy link
Contributor

jijotj commented Jan 19, 2023

Oh cool, I chose kingpin because it was already used in the Loki ecosystem by logcli. Thanks for suggesting a compromise, I'll raise a PR to do just that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/loki-canary size/L type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants