Skip to content

Conversation

@ArtificialOwl
Copy link
Member

@ArtificialOwl ArtificialOwl commented May 14, 2025

Allow a rename of config keys, with a migration of linked values
also, using old config key will returns value from new config key

migration can also be initiated using:

./occ config:list --migrate [<appId>]
  • tests

adding to CoreConfigLexicon.php the entry:

     new ConfigLexiconEntry('key123', ValueType::STRING, '', lazy: true, rename: 'key000'),

will returns:

$ ./occ config:app:get --details core key000
  - app: core
  - key: key123
  - value: my_value_in_database
  - type: string
  - lazy: true
  - sensitive: false

@ArtificialOwl ArtificialOwl requested a review from a team as a code owner May 14, 2025 18:12
@ArtificialOwl ArtificialOwl requested review from come-nc, provokateurin and yemkareems and removed request for a team May 14, 2025 18:12
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Looks promising, I can’t wait for the tests.

@ArtificialOwl ArtificialOwl force-pushed the feat/noid/lexicon-migrate-keys branch 2 times, most recently from 5810acc to 18b732f Compare May 15, 2025 10:54
@codecov
Copy link

codecov bot commented May 15, 2025

❌ We are unable to process any of the uploaded JUnit XML files. Please ensure your files are in the right format.

@ArtificialOwl ArtificialOwl force-pushed the feat/noid/lexicon-migrate-keys branch 3 times, most recently from bcdd3bd to 5a8e422 Compare May 15, 2025 16:34
@ArtificialOwl
Copy link
Member Author

@nickvergessen @susnux

what you are looking for is:

	new ConfigLexiconEntry('new_key', ValueType::BOOL, rename: 'old_key', options: ConfigLexiconEntry::RENAME_INVERT_BOOLEAN),

@ArtificialOwl ArtificialOwl marked this pull request as draft May 16, 2025 09:38
@ArtificialOwl ArtificialOwl force-pushed the feat/noid/lexicon-migrate-keys branch from 5a8e422 to 52d1666 Compare May 16, 2025 10:54
@ArtificialOwl ArtificialOwl added the 2. developing Work in progress label May 16, 2025
@ArtificialOwl ArtificialOwl added this to the Nextcloud 32 milestone May 16, 2025
@ArtificialOwl ArtificialOwl force-pushed the feat/noid/lexicon-migrate-keys branch 2 times, most recently from bf79458 to 9fdc9a2 Compare May 16, 2025 15:45
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

I think "alias" is not a good name, as it doesn't make clear the old keys are deprecated.

@ArtificialOwl ArtificialOwl force-pushed the feat/noid/lexicon-migrate-keys branch 7 times, most recently from a779676 to 00b02fe Compare June 10, 2025 19:39
@ArtificialOwl ArtificialOwl force-pushed the feat/noid/lexicon-migrate-keys branch 2 times, most recently from b4803fa to c9067eb Compare June 13, 2025 16:35
@ArtificialOwl ArtificialOwl marked this pull request as ready for review June 13, 2025 16:55
@ArtificialOwl ArtificialOwl force-pushed the feat/noid/lexicon-migrate-keys branch from c9067eb to e104f78 Compare June 17, 2025 17:02
@ArtificialOwl ArtificialOwl requested a review from come-nc June 18, 2025 12:39
@ArtificialOwl ArtificialOwl force-pushed the feat/noid/lexicon-migrate-keys branch from e104f78 to 1b42dad Compare June 19, 2025 09:09
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm a bit confused why almost all changes to AppConfig/UserConfig are duplicate. I am pretty sure, that over time the implementations will deviate more and more and this calls for painful bugs. Would it be possible to have a base class with the shared logic and have each of them inherit from it? @ArtificialOwl

@ArtificialOwl
Copy link
Member Author

LGTM, but I'm a bit confused why almost all changes to AppConfig/UserConfig are duplicate. I am pretty sure, that over time the implementations will deviate more and more and this calls for painful bugs. Would it be possible to have a base class with the shared logic and have each of them inherit from it? @ArtificialOwl

Could be, but not sure it is the purpose of this PR

@provokateurin
Copy link
Member

You are right, but it makes the problem worse and I feel like there will be no improvement later if we ignore it now.

@ArtificialOwl
Copy link
Member Author

I am all for avoiding duplicate code but, from my point of view, there is enough difference between AppConfig and UserConfig that trying to link them together will make everything more complex:

  • AppConfig was made before the ability to use Enums,
  • sensitivity is not set as a type bit anymore in UserConfig,
  • cache also includes the userId dimension,

I already added ConfigManager in that PR that contains few tools used by both classes.

What we can do is having a look at the final version of the code when moving the feature from unstable/ to public/ - that I would like to request in the next 2 weeks.

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Ok, as long as this is not forgotten I'm fine with it.

@ArtificialOwl ArtificialOwl force-pushed the feat/noid/lexicon-migrate-keys branch from 1b42dad to d860cfd Compare June 24, 2025 13:11
@ArtificialOwl ArtificialOwl enabled auto-merge June 24, 2025 13:45
@ArtificialOwl ArtificialOwl merged commit d161e07 into master Jun 24, 2025
204 of 206 checks passed
@ArtificialOwl ArtificialOwl deleted the feat/noid/lexicon-migrate-keys branch June 24, 2025 13:54
@skjnldsv skjnldsv mentioned this pull request Aug 19, 2025
@skjnldsv skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants