New default path for config file #4301
Conversation
codebien
force-pushed
the
config-file-tests
branch
from
a9e3047 to
4fe44f4
Compare
codebien
force-pushed
the
k6-config-path
branch
from
d93279c to
7588059
Compare
codebien
force-pushed
the
k6-config-path
branch
from
7588059 to
91710b9
Compare
codebien
force-pushed
the
k6-config-path
branch
from
91710b9 to
7e205bb
Compare
codebien
force-pushed
the
k6-config-path
branch
from
7e205bb to
43a3cd9
Compare
codebien
requested review from
mstoykov,
joanlopez,
a team and
inancgumus
and removed request for
a team and
mstoykov
There was a problem hiding this comment.
Overall it looks mostly fine, except for a few minor things I pointed out during the review.
However, I have the general doubt around when should we warn users and when should we perform the configuration migration. Isn't file disk configuration also used for other commands like run, archive or cloud run?
Why do we only consider it for k6 cloud login?
Do users normally don't store anything beyond cloud configuration? 🤔
| } | ||
|
|
||
| func readLegacyDiskConfig(gs *state.GlobalState) (Config, error) { | ||
| // CHeck if the legacy config exists in the supplied filesystem |
There was a problem hiding this comment.
| // CHeck if the legacy config exists in the supplied filesystem | |
| // Check if the legacy config exists in the supplied filesystem |
internal/cmd/config.go
Outdated
| gs.Logger.Warn("The configuration file has been found on the old path. " + | ||
| "Please, run again `k6 cloud login` or `k6 login` commands to migrate to the new path. " + | ||
| "If you already migrated it manually, then remove the file from the old path.\n\n") |
There was a problem hiding this comment.
I think the warning is fine 👌🏻 but I'm not sure whether we should try to use the "new" config file path, if exists, as the default, or not. In such case, we may need to slightly adjust the message.
There was a problem hiding this comment.
For instance, some of the scenarios that trigger those doubts in my mind are those in where the new configuration file path exists and is correct, while the old one also exists, but there are issues with them.
In all those cases, an error would be triggered, with no mention to migration. Perhaps too tricky.
There was a problem hiding this comment.
I'm considering to reverse the flow, where we always check the new default before, and only eventually we check the old one. I didn't do that before because the code is a bit tricky in the current version because we are silencing the Not Found error. But I realized it's better to stretch a bit the code to fit it instead of yielding this cognitive load on the user.
internal/cmd/config.go
Outdated
| return Config{}, errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig) | ||
| } else { | ||
| gs.Logger.Warn("The configuration file has been found on the old path. " + | ||
| "Please, run again `k6 cloud login` or `k6 login` commands to migrate to the new path. " + |
There was a problem hiding this comment.
| "Please, run again `k6 cloud login` or `k6 login` commands to migrate to the new path. " + | |
| "Please, run again `k6 cloud login` commands to migrate to the new path. " + |
As k6 login has been deprecated, I think it's better to not mention it. Right? 🤔
There was a problem hiding this comment.
If a user has a config file because it used k6 login influxdb but they don't have the Cloud they won't be able to migrate anymore. Do you think we should not support them?
| } | ||
|
|
||
| gs.Logger.Infof("Note, the configuration file has been migrated "+ | ||
| "from old default path (%q) to the new version (%q).\n\n", legacyFpath, gs.Flags.ConfigFilePath) |
There was a problem hiding this comment.
| "from old default path (%q) to the new version (%q).\n\n", legacyFpath, gs.Flags.ConfigFilePath) | |
| "from the old default path (%q) to the new one (%q).\n\n", legacyFpath, gs.Flags.ConfigFilePath) |
|
Hey @joanlopez, thanks for reviewing 👋
I applied mostly what was suggested here #2508 (comment). It makes sense for me because it keeps the same operations' flows. k6 writes the config on In addition, re-reading Ned's comment, I realized I'm applying a reversed logic on the reading path, where we read before the legacy config and only after the new. But there isn't really a requirement for doing it, we can just directly read the new path and only if it doesn't exist then do an additional attempt on the legacy. Does it make sense for you? The goal here is to provide the best UX so if you have any suggestion to improve it, I'm happy to evaluate them. |
|
@inancgumus @joanlopez I have a doubt. During the migration, do you think should we delete the old directories ( |
If I understand the approach here correctly, it feels safer and more user-friendly to leave the old directory intact by default. Automatically deleting it can cause unintended data loss or break workflows if the user relies on that directory in ways we haven't anticipated. We can allow users to remove it themselves once they confirm everything works in the new location. |
What?
It changes the default path for config file to
.config/k6/config.jsonfrom the legacyloadimpactversion.Why?
Check #2508
Related PR(s)/Issue(s)
Closes #2508