Skip to content

Remove legacy config path migration and fallbacks - #5609

Merged
inancgumus merged 1 commit into
grafana:v2from
janHildebrandt98:cleanup/remove-legacy-config-migration
Feb 26, 2026
Merged

Remove legacy config path migration and fallbacks#5609
inancgumus merged 1 commit into
grafana:v2from
janHildebrandt98:cleanup/remove-legacy-config-migration

Conversation

@janHildebrandt98

@janHildebrandt98 janHildebrandt98 commented Feb 4, 2026

Copy link
Copy Markdown
Contributor

What?

Removed all code responsible for handling migration from the old legacy config file path to the new one.
Also dropped fallback logic.

Why?

The migration/fallback was intended to stick around for v1 only and, as a breaking change with v2, we are now intentionally removing it to simplify the code.

Checklist

  • I have performed a self-review of my code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added tests for my changes.
  • I have run linter and tests locally (make check) and all pass.

Checklist: Documentation (only for k6 maintainers and if relevant)

Please do not merge this PR until the following items are filled out.

  • I have added the correct milestone and labels to the PR.
  • I have updated the release notes: link
  • I have updated or added an issue to the k6-documentation: grafana/k6-docs#NUMBER if applicable
  • I have updated or added an issue to the TypeScript definitions: grafana/k6-DefinitelyTyped#NUMBER if applicable

Related PR(s)/Issue(s)

Closes #5572

@janHildebrandt98
janHildebrandt98 requested a review from a team as a code owner February 4, 2026 19:37
@janHildebrandt98
janHildebrandt98 requested review from ankur22 and inancgumus and removed request for a team February 4, 2026 19:37
@inancgumus
inancgumus requested a review from codebien February 4, 2026 20:57
@janHildebrandt98
janHildebrandt98 temporarily deployed to azure-trusted-signing February 4, 2026 21:04 — with GitHub Actions Inactive
@janHildebrandt98
janHildebrandt98 temporarily deployed to azure-trusted-signing February 4, 2026 21:06 — with GitHub Actions Inactive
@janHildebrandt98

janHildebrandt98 commented Feb 5, 2026

Copy link
Copy Markdown
Contributor Author

@codebien
I am a little confused about the failed tests. After a brief investigation it seems like it is not touching the code i changed. Also on the other environments (as well as locally on my system) the tests seem to be green.

Is there something I am missing?

Edit:
After some more investigation this seems to happen because of some state/cache in json.Unmarshal() in go tip.
The race detector flags concurrent access to that cache because the tests are concurrently calling json.Unmarshal(). I don't think this is connected to this PR.

@codebien

codebien commented Feb 5, 2026

Copy link
Copy Markdown
Contributor

@janHildebrandt98 the fix is now merged, consider to rebase your pull request.

@janHildebrandt98
janHildebrandt98 force-pushed the cleanup/remove-legacy-config-migration branch from c7c6a6e to 552ed68 Compare February 5, 2026 17:06
@janHildebrandt98
janHildebrandt98 force-pushed the cleanup/remove-legacy-config-migration branch from 552ed68 to dd87944 Compare February 5, 2026 17:28
@janHildebrandt98

janHildebrandt98 commented Feb 5, 2026

Copy link
Copy Markdown
Contributor Author

@codebien
After a little Git adventure I noticed that rebasing is not a good idea since we want to merge into v2 and there are a bunch of differences to master, were you merged the fix.
I just removed my equivalent change so we only have the removal of the migration stuff in here now. I guess there will be a sync between v2 and master at some point?
Git history of this PR is kind of a mess now :D

@janHildebrandt98

Copy link
Copy Markdown
Contributor Author

@codebien
Do I need to do something here or can we proceed?

@codebien codebien left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey thanks for you contribution!

assert.ErrorIs(t, err, fs.ErrNotExist)
}

func TestLoadConfig(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We want to keep the testing for the current path. We should only drop the ones related to the legacy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

TestLoadConfig() tested the loadConfig() method which I removed because it was just a wrapper around readDiskConfig() to decide from where to load the config (legacy vs. new).
Thats why I think the whole test is obsolete now. The logic is sufficiently tested by the tests for readDiskConfig() which is now called everywhere directly.

@codebien

Copy link
Copy Markdown
Contributor

After a little Git adventure I noticed that rebasing is not a good idea since we want to merge into v2 and there are a bunch of differences to master, were you merged the fix.

Yep, we are aware of it. I'm going to rebase v2, as soon as possible.

@codebien

codebien commented Feb 16, 2026

Copy link
Copy Markdown
Contributor

v2 is now in sync with master, you can rebase your branch

@janHildebrandt98
janHildebrandt98 force-pushed the cleanup/remove-legacy-config-migration branch from 23f0fec to a1c4b1c Compare February 17, 2026 16:39
@janHildebrandt98

janHildebrandt98 commented Feb 17, 2026

Copy link
Copy Markdown
Contributor Author

@codebien
I rebased on v2 and answered your comment.
Thanks you for your time to review the PR.

@inancgumus
inancgumus merged commit 8f64943 into grafana:v2 Feb 26, 2026
49 checks passed
@janHildebrandt98
janHildebrandt98 deleted the cleanup/remove-legacy-config-migration branch March 4, 2026 09:22
@joanlopez joanlopez added this to the v1.7.0 milestone Mar 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants