Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(config/validation): improve validation of global options #25218

Merged
merged 32 commits into from Feb 21, 2024

Conversation

RahulGautamSingh
Copy link
Collaborator

@RahulGautamSingh RahulGautamSingh commented Oct 15, 2023

Changes

Add validation for globalOptions.

This PR changes the behaviour of config-validator when in comes to validating the file config. Till now we couldn't validate the global options inside file config fully ... even after adding logic to warn for any global option in repo config. We used to get invalid config option error instead if a proper validation erro... This has been fixed and now we can valiate all the config options, based on their types, and allowedValues (where needed).

This doesn't effect the existing behaviour of repo config validation.

I decided to create a new function to validate the global options even though it was possible to modify the existing function for our use case, because we want to warn for any validation errors and not throw errors. Since the existing function is designed mainly for errors adding warnings for the global options would've made it less redable and more nested.

Context

Needed For: #26379

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

Copy link
Collaborator

@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 expand the description as this is too generic/non-descriptive. What is this PR changing and why?

lib/config/validation.ts Outdated Show resolved Hide resolved
lib/config/validation.ts Outdated Show resolved Hide resolved
RahulGautamSingh and others added 2 commits October 16, 2023 16:22
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
lib/config-validator.ts Outdated Show resolved Hide resolved
lib/config/validation.spec.ts Outdated Show resolved Hide resolved
@rarkins
Copy link
Collaborator

rarkins commented Oct 17, 2023

What is the expected difference before/after this PR?

I just ran with an invalid option in config.js and did not get any warning or error

@rarkins
Copy link
Collaborator

rarkins commented Oct 17, 2023

The log:

❯ pnpm start --dry-run=extract renovate-reproductions/24460

> renovate@0.0.0-semantic-release prestart /Users/rhys/src/github/renovatebot/renovate
> run-s 'generate:*'


> renovate@0.0.0-semantic-release generate:imports /Users/rhys/src/github/renovatebot/renovate
> node tools/generate-imports.mjs

generating imports
> data/debian-distro-info.json
> data/kubernetes-api.json5
> data/node-js-schedule.json
> data/ubuntu-distro-info.json
> node_modules/emojibase-data/en/shortcodes/github.json
generating hashes

> renovate@0.0.0-semantic-release start /Users/rhys/src/github/renovatebot/renovate
> ts-node lib/renovate.ts "--dry-run=extract" "renovate-reproductions/24460"

DEBUG: Using RE2 as regex engine
DEBUG: Parsing configs
DEBUG: Checking for config file in /Users/rhys/src/github/renovatebot/renovate/config.js
DEBUG: Enabling debug logging to tmp/logs/2023-10-17T11:30:39.291Z.log
DEBUG: File config
       "config": {
         "platform": "github",
         "token": "***********",
         "binarySource": "docker",
         "baseDir": "/tmp/renovate",
         "onboardingConfig": {"extends": ["config:recommended"]},
         "prHourlyLimit": 0,
         "prConcurrentLimit": 0,
         "repositories": ["renovate-reproductions/24460"],
         "force": {"schedule": null},
         "logFile": "tmp/logs/2023-10-17T11:30:39.291Z.log",
         "allowCustomCrateRegistries": "nope"
       }
DEBUG: CLI config
       "config": {"repositories": ["renovate-reproductions/24460"], "dryRun": "extract"}
DEBUG: Env config
       "config": {"hostRules": []}
DEBUG: Combined config
       "config": {
         "platform": "github",
         "token": "***********",
         "binarySource": "docker",
         "baseDir": "/tmp/renovate",
         "onboardingConfig": {"extends": ["config:recommended"]},
         "prHourlyLimit": 0,
         "prConcurrentLimit": 0,
         "repositories": ["renovate-reproductions/24460"],
         "force": {"schedule": null},
         "logFile": "tmp/logs/2023-10-17T11:30:39.291Z.log",
         "allowCustomCrateRegistries": "nope",
         "hostRules": [],
         "schedule": null,
         "dryRun": "extract"
       }
DEBUG: Enabling forkProcessing while in non-autodiscover mode
DEBUG: Found valid git version: 2.38.1
DEBUG: Using default github endpoint: https://api.github.com/
DEBUG: Platform config
       "platformConfig": {
         "hostType": "github",
         "endpoint": "https://api.github.com/",
         "isGHApp": false,
         "isGhe": false,
         "userDetails": {
           "username": "renovate-tests",
           "name": "Renovate Testing",
           "id": 29705663
         },
         "userEmail": "renovate-tests@redgc.com"
       },
       "renovateUsername": "renovate-tests"
DEBUG: Using platform gitAuthor: Renovate Testing <renovate-tests@redgc.com>

My expectation was that the "allowCustomCrateRegistries": "nope" field would trigger a warning.

@RahulGautamSingh
Copy link
Collaborator Author

What is the expected difference before/after this PR?

I just ran with an invalid option in config.js and did not get any warning or error

This is because we do not validate file config in repo runs.
To catch the errors you need to run config-validator

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.

we should get this done

lib/config-validator.ts Outdated Show resolved Hide resolved
lib/config-validator.ts Outdated Show resolved Hide resolved
lib/config-validator.ts Outdated Show resolved Hide resolved
lib/config-validator.ts Outdated Show resolved Hide resolved
lib/config-validator.ts Outdated Show resolved Hide resolved
lib/config/validation.spec.ts Outdated Show resolved Hide resolved
lib/config/validation.ts Outdated Show resolved Hide resolved
@RahulGautamSingh RahulGautamSingh changed the title feat(config/validation): basic file config validation feat(config): basic file config validation Dec 21, 2023
@RahulGautamSingh

This comment was marked as outdated.

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.

we should refactor validation to use same style as config migration for easier future additions and easier testing.

lib/config/validation.ts Outdated Show resolved Hide resolved
lib/config/validation.ts Outdated Show resolved Hide resolved
lib/config/validation.ts Outdated Show resolved Hide resolved
@RahulGautamSingh RahulGautamSingh changed the title feat(config/validation): validate globalOptions in file config feat(config/validation): add validation for global options Feb 13, 2024
lib/config/validation.ts Outdated Show resolved Hide resolved
lib/config/validation.ts Outdated Show resolved Hide resolved
lib/config/validation.ts Show resolved Hide resolved
Copy link
Collaborator

@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.

Is this a feature or a fix? There's seems to have been some logic here already which is improved/fixed

@RahulGautamSingh
Copy link
Collaborator Author

RahulGautamSingh commented Feb 21, 2024

Is this a feature or a fix? There's seems to have been some logic here already which is improved/fixed

Adding feat scope was a mistake. It should be the fix scope.

It does 2 jobs:

  1. It introduces validation for the global options which were only validated based on type.
    ie. binarySource was validated as string and any string was allowed. But now we only allow the values global|docker|hermit|install.

  2. It fixes the wrong validation message we used to get when some globalOptions.
    eg. If cacheTtlOverride was present in file config.
    We used to get "message": "Invalid configuration option: cacheTtlOverride.string". Now we get a proper validation message.

Basically we improve the validation, so a fix.

@RahulGautamSingh RahulGautamSingh changed the title feat(config/validation): add validation for global options fix(config/validation): improve validation of global options Feb 21, 2024
@viceice viceice added this pull request to the merge queue Feb 21, 2024
Merged via the queue into renovatebot:main with commit 7a57d88 Feb 21, 2024
34 checks passed
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 37.203.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@MindTooth
Copy link
Contributor

Not to bump this, but this kinda broke stuff, and I'm not complete sure how to fix this. E.g.:

     "warnings": [
         {
           "topic": "Configuration Error",
           "message": "Configuration option `hostRules[0].token` should be a string."
         },
         {
           "topic": "Configuration Error",
           "message": "Invalid `onboardingConfig.onboardingConfig.extends` configuration: value must be a string."
         }
       ]

Taken the example from https://docs.renovatebot.com/examples/self-hosting/#usage even produces an error:

 WARN: Found errors in configuration
       "file": "config.js",
       "warnings": [
         {
           "topic": "Configuration Error",
           "message": "Invalid `onboardingConfig.onboardingConfig.extends` configuration: value must be a string."
         }
       ]

@RahulGautamSingh
Copy link
Collaborator Author

RahulGautamSingh commented Feb 21, 2024

Not sure about the hostRules error, I couldn't reproduce it. Can you share you config for it?

The onboardingConfig one was reproduced. It is being validated as a plainObject ie, string key value pair. I will add a fix for it. Thanks.

@MindTooth
Copy link
Contributor

  hostRules: [
    {
      matchHost: "gitlab.<the rest>",
      token: process.env.RENOVATE_TOKEN,
    },
  ],

That might however be my own error? 🤔

@rarkins
Copy link
Collaborator

rarkins commented Feb 21, 2024

Is this from when you run it with the validator?

@MindTooth
Copy link
Contributor

Is this from when you run it with the validator?

Correct. And it has now broken the pipeline, meaning the template from GitLab won't run.

@rarkins
Copy link
Collaborator

rarkins commented Feb 21, 2024

I think I can reproduce one of these. If you have a config.js, and it has a host rules entry with token pointing to an env variable, and that env variable doesn't exist when you run the validator, then the token=undefined and the validator will error. If instead the env variable is defined/valid then you don't get an error.

@rarkins
Copy link
Collaborator

rarkins commented Feb 21, 2024

@MindTooth please create a new github discussion to continue this topic

@HonkingGoose
Copy link
Collaborator

Do we need to fix the documentation or the code for the example:

Usage

The following example uses the Renovate CLI tool, which you can install by running npm i -g renovate.

If running your own Renovate bot then you will need a user account that Renovate will run as.
We recommend you create and use a dedicated account for the bot, e.g. name it renovate-bot if on your own instance.
Create and save a PAT for this account.

Create a Renovate config file, for example:

module.exports = {
  endpoint: 'https://self-hosted.gitlab/api/v4/',
  token: '**gitlab_token**',
  platform: 'gitlab',
  onboardingConfig: {
    extends: ['config:recommended'],
  },
  repositories: ['username/repo', 'orgname/repo'],
};

Here change the logFile and repositories to something appropriate.
Also replace gitlab-token value with the one created during the previous step.

If you're running against GitHub Enterprise Server, then change the gitlab values in the example to the equivalent GitHub ones.

You can save this file as anything you want and then use the RENOVATE_CONFIG_FILE environment variable to tell Renovate where to find it.

Most people use cron to schedule when Renovate runs, usually on an hourly schedule.

#!/bin/bash

export PATH="/home/user/.yarn/bin:/usr/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:$PATH"
export RENOVATE_CONFIG_FILE="/home/user/renovate-config.js"
export RENOVATE_TOKEN="**some-token**" # GitHub, GitLab, Azure DevOps
export GITHUB_COM_TOKEN="**github-token**" # Delete this if using github.com

# Renovate
renovate

Save the script file, and run the script manually.
Only add the script to cron after you checked it works.

!!! note
The GitHub.com token as an environment variable is needed to fetch changelogs that are usually hosted on github.com.
You don't need to add it if you are already running the bot against github.com, but you do need to add it if you're using GitHub Enterprise Server, GitLab, Azure DevOps, or Bitbucket.

@MindTooth
Copy link
Contributor

@MindTooth please create a new github discussion to continue this topic

#27468

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.

None yet

6 participants