Skip to content

Conversation

@RahulGautamSingh
Copy link
Collaborator

@RahulGautamSingh RahulGautamSingh commented Aug 27, 2023

Changes

  • Added all globalOnly=true options to RepoGlobalConfig
  • Added all the options to GlobalConfig.OPTIONS
  • Added a test to confirm GlobalConfig.OPTIONS includes all options with globalOnly=true for consistency in future

Context

Closes: #9603
Doing this as a part of #23868

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@RahulGautamSingh RahulGautamSingh changed the title test: make sure GlobalConfig.OPTIONS includes all options with globalOnly=true test: update GlobalConfig.OPTIONS Aug 27, 2023
@RahulGautamSingh RahulGautamSingh marked this pull request as draft August 27, 2023 16:12
@RahulGautamSingh RahulGautamSingh removed the request for review from zharinov August 27, 2023 16:12
@RahulGautamSingh RahulGautamSingh marked this pull request as ready for review August 28, 2023 00:28
@RahulGautamSingh RahulGautamSingh changed the title test: update GlobalConfig.OPTIONS test: update GlobalConfig.OPTIONS and RepoGlobalConfig Aug 28, 2023
@RahulGautamSingh RahulGautamSingh changed the title test: update GlobalConfig.OPTIONS and RepoGlobalConfig fix(types): update GlobalConfig.OPTIONS and RepoGlobalConfig Aug 28, 2023
@RahulGautamSingh RahulGautamSingh changed the title fix(types): update GlobalConfig.OPTIONS and RepoGlobalConfig fix(types): add all globalOnly=true options to RepoGlobalConfig Aug 28, 2023
Comment on lines +31 to +33
'autodiscover',
'autodiscoverFilter',
'autodiscoverTopics',
Copy link
Contributor

Choose a reason for hiding this comment

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

Will ones like this still be deleted from config at an early stage, and not copied available via GlobalConfig.get()?

Copy link
Collaborator Author

@RahulGautamSingh RahulGautamSingh Aug 28, 2023

Choose a reason for hiding this comment

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

These options will now be available via GlobalConfig.get().

They weren't available previously, because they were present in GlobalConfig.OPTIONS and were ignored by GlobalConfig,set(). The logic thought these weren't global config options., therefore instead of setting it to GlobalConfig it just returned it and then they got added to the repoConfig (renovate.json).

This happens here:

let config = GlobalConfig.set(
applySecretsToConfig(repoConfig, undefined, false)
);

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we ever want to GlobalConfig.get() them. So the question is whether this is a bad thing or not.

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

needs real tests, I think it also needs more code changes to use the global config get function. otherwise this probably breaks existing behavior.

@RahulGautamSingh
Copy link
Collaborator Author

RahulGautamSingh commented Aug 28, 2023

needs real tests, I think it also needs more code changes to use the global config get function. otherwise this probably breaks existing behavior.

Yes. Should I divide this into multiple PRs each targeting a related group of options
eg. autodiscover, autodiscoverFilter, autoDiscoverTopic

'force',
'forceCli',
'forkOrg',
'forkToken',
Copy link
Contributor

Choose a reason for hiding this comment

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

No, this is definition not right

'requireConfig',
'secrets',
'skipInstalls',
'token',
Copy link
Contributor

Choose a reason for hiding this comment

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

Not right

Copy link
Contributor

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

Please go back to the issue and discuss approaches there instead. This does not seem like the right approach

@rarkins rarkins closed this Aug 29, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add tests for "config/admin.ts"

3 participants