valkey icon indicating copy to clipboard operation
valkey copied to clipboard

Published 20 hours ago • verified publisher icon valkey-io

Remove trademarked wording on configuration file and individual configs

Open hpatro opened this issue 10 months ago • 16 comments

Remove trademarked wording on configuration layer.

Following changes for release notes:

  1. Update to valkey.conf
  2. config changes
    1. pidfile - /var/run/valkey_6379.pid

hpatro avatar Mar 25 '24 23:03 hpatro

@madolson Tried making the comments generic. PTAL.

hpatro avatar Mar 26 '24 05:03 hpatro

@placeholderkv/core-team Shall we maintain an issue with list of new changes or should we just attach a label breaking-changes to the PR(s), which will be reviewed by the release owner/core-team later on to generate a summary?

hpatro avatar Mar 26 '24 07:03 hpatro

But some global names like pidfile and socket file would need to be more unique (as commented) and what about the binaries? It'd be silly to just call them server, cli, benchmark, right? I think we need a name here.

I think we should keep pid and socket files to be specific to the engine. I agree with moving config -> server.conf. The server/cli/benchmark should have some prefix, but that will depend on the name we end up on. I agree with -DName approach for now.

madolson avatar Mar 27 '24 03:03 madolson

@zuiderkwast @PingXie Thanks for the review. Could you PTAL once again?

I've added a compile time param SERVER_DEF_NAME to help defining the server name.

hpatro avatar Mar 29 '24 20:03 hpatro

@zuiderkwast @PingXie Thanks for the review. Could you PTAL once again?

Thanks for reducing the scope of this pr.

I think we still need a uber decision/guidance on file name changes:

  1. keep the old names
  2. change but symlink
  3. change and break

PingXie avatar Mar 29 '24 20:03 PingXie

@zuiderkwast @PingXie Thanks for the review. Could you PTAL once again?

Thanks for reducing the scope of this pr.

I think we still need a uber decision/guidance on file name changes:

  1. keep the old names

  2. change but symlink

  3. change and break

I'm inclined with 2.

hpatro avatar Mar 29 '24 21:03 hpatro

@zuiderkwast @PingXie Thanks for the review. Could you PTAL once again?

Thanks for reducing the scope of this pr. I think we still need a uber decision/guidance on file name changes:

  1. keep the old names
  2. change but symlink
  3. change and break

I'm inclined with 2.

I am fine with option 2 as well. Can you propose the changes in this pr? Normally I would be in favor of smaller PRs but since we will need to cherry-pick these changes into a 7.2 branch, I think it is important that every pr is self-contained so we don't have to deal with dependency and order.

PingXie avatar Mar 29 '24 21:03 PingXie

@placeholderkv/core-team Shall we maintain an issue with list of new changes or should we just attach a label breaking-changes to the PR(s), which will be reviewed by the release owner/core-team later on to generate a summary?

I don't see a follow up discussion on this questions. Tagging is fine. No need to denormalize the information. I prefer light processes.

PingXie avatar Mar 29 '24 22:03 PingXie

@PingXie I've created a symlink (source - valkey.conf target - redis.conf), let me know your thoughts.

hpatro avatar Mar 30 '24 00:03 hpatro

@placeholderkv/core-team Shall we maintain an issue with list of new changes or should we just attach a label breaking-changes to the PR(s), which will be reviewed by the release owner/core-team later on to generate a summary?

I don't see a follow up discussion on this questions. Tagging is fine. No need to denormalize the information. I prefer light processes.

I guess we aren't breaking anything with this PR anymore. Agreed let's keep the process light. Release owner would need to visit the PR(s) marked with breaking-change to generate the release note.

hpatro avatar Mar 30 '24 00:03 hpatro

@PingXie @zuiderkwast

  • Sticking to valkey.conf to avoid any ambiguity (getting rid of generic configuration file comments).
  • Moving the SERVER_DEF_NAME out of this PR and avoiding any changes around syslog-ident config.

hpatro avatar Apr 01 '24 17:04 hpatro

@PingXie @zuiderkwast

  • Sticking to valkey.conf to avoid any ambiguity (getting rid of generic configuration file comments).
  • Moving the SERVER_DEF_NAME out of this PR and avoiding any changes around syslog-ident config.

yes. one more:

  • create symlinks in "make install" only; don't check in symlinks

and I think we are good to go

PingXie avatar Apr 01 '24 19:04 PingXie

@PingXie PTAL.

hpatro avatar Apr 01 '24 20:04 hpatro

@PingXie @zuiderkwast

  • Sticking to valkey.conf to avoid any ambiguity (getting rid of generic configuration file comments).
  • Moving the SERVER_DEF_NAME out of this PR and avoiding any changes around syslog-ident config.

yes. one more:

  • create symlinks in "make install" only; don't check in symlinks

and I think we are good to go

Note: I've placed the symlink under the same directory as valkey.conf.

hpatro avatar Apr 01 '24 20:04 hpatro

@zuiderkwast @PingXie @hwware PTAL.

@hwware Will address the other feedback in a separate PR.

hpatro avatar Apr 02 '24 19:04 hpatro

Moving the config related changes to valkey specific term.

hpatro avatar Apr 02 '24 19:04 hpatro

I applied some changes I think you missed in the jungle of comments.

Let's merge this?

Once DCO pass, pls meger it. LGTM

hwware avatar Apr 03 '24 14:04 hwware

@zuiderkwast @PingXie @hwware Could we do a squash and merge through github and add sign off in that commit?

hpatro avatar Apr 03 '24 17:04 hpatro

Try this

https://stackoverflow.com/questions/5189560/how-do-i-squash-my-last-n-commits-together

PingXie avatar Apr 03 '24 17:04 PingXie

Try this

https://stackoverflow.com/questions/5189560/how-do-i-squash-my-last-n-commits-together

ofc I can do that, asking if we could avoid it and directly go for the squash/merge in github.

hpatro avatar Apr 03 '24 17:04 hpatro

There are some merge conflicts with unstable. performing the squash locally and force pushing here.

hpatro avatar Apr 03 '24 17:04 hpatro

@valkey-io/core-team Sadly the co-authored-by commit message got gobbled up. Please add it while you merge.

hpatro avatar Apr 03 '24 17:04 hpatro

@hpatro Is the top comment updated? No other changes than config file and pidfile? (It's not the default but only a suggested pidfile in the conf file.)

zuiderkwast avatar Apr 03 '24 17:04 zuiderkwast

co-authored-by who exactly?

I think it doesn't matter in this case. We can skip it.

zuiderkwast avatar Apr 03 '24 17:04 zuiderkwast

@hpatro Is the top comment updated? No other changes than config file and pidfile? (It's not the default but only a suggested pidfile in the conf file.)

The only meaningful change is rename of redis.conf to valkey.conf. Updated accordingly.

co-authored-by who exactly?

I think it doesn't matter in this case. We can skip it.

It was you :)

hpatro avatar Apr 03 '24 17:04 hpatro

If people use redis.conf (valkey.conf) in scripts and maybe change something using sed or so, they would be affected by the pid file. In the commit message when merging I mentioned it:

Remove trademarked wording on configuration layer.

Following changes for release notes:

1. Rename redis.conf to valkey.conf
2. Pre-filled config in the template config file: Changing pidfile to `/var/run/valkey_6379.pid`

zuiderkwast avatar Apr 03 '24 20:04 zuiderkwast