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

Provide better feedback when gitpod.yml is invalid #4844

Merged
merged 2 commits into from
Jul 19, 2021

Conversation

csweichel
Copy link
Contributor

fixes #4828

This PR adds a proper failure mode for invalid gitpod.yml configuration. Instead of silently falling back to the default config, it surfaces the error and offers the choice to start with the default config.

How to test

  1. https://csweichel-provide-better-feedback-4828.staging.gitpod-dev.com/#https://github.com/gitpod-io/gitpod-test-repo/tree/broken-gitpod-config

@@ -47,17 +47,91 @@ export class ConfigProvider {
let configBasePath = '';
Copy link
Member

Choose a reason for hiding this comment

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

nit: This can be moved into the try

if (AdditionalContentContext.is(commit) && commit.additionalFiles['.gitpod.yml']) {
const customConfigString = commit.additionalFiles['.gitpod.yml'];
const parseResult = this.gitpodParser.parse(customConfigString);
customConfig = parseResult.config;
customConfig._origin = 'additional-content';
if (parseResult.validationErrors) {
log.error(logContext, `Invalid config. Errors are ${parseResult.validationErrors.join(',')}`, { repoCloneUrl: commit.repository.cloneUrl, revision: commit.revision, customConfigString });
const err = new InvalidGitpodYMLError(parseResult.validationErrors);
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -90,7 +164,10 @@ export class ConfigProvider {
customConfig = parseResult.config
await this.fillInDefaultLocations(customConfig, inferredConfig);
if (parseResult.validationErrors) {
log.error(logContext, `Skipping invalid config. Errors are ${parseResult.validationErrors.join(',')}`, { repoCloneUrl: commit.repository.cloneUrl, revision: commit.revision, customConfigString });
const err = new InvalidGitpodYMLError(parseResult.validationErrors);
Copy link
Member

Choose a reason for hiding this comment

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

👍

(it's a bit hard to parse the diff, but I understood that these three changes are the only relevant ones, the rest seems to be a move)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Also note the demotion of log prio. If this fails here it's a user input issue, not a system error

@@ -47,17 +47,91 @@ export class ConfigProvider {
let configBasePath = '';
try {
let customConfig: WorkspaceConfig | undefined;

if (!WithDefaultConfig.is(commit)) {
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -310,3 +344,17 @@ export class ConfigProvider {
}

}

export class InvalidGitpodYMLError extends Error {
Copy link
Member

Choose a reason for hiding this comment

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

We have quite some classes like this around. Would be awesome if we found a way to generalize this approach! 👍

@geropl
Copy link
Member

geropl commented Jul 19, 2021

/werft run

👍 started the job as gitpod-build-csweichel-provide-better-feedback-4828.4

Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Code looks good and tested the validation detection: ✔️

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 7d683b691369002e0e2b93e724ca7523ce21e6f3

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csweichel, geropl

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit 84cc8a1 into main Jul 19, 2021
@roboquat roboquat deleted the csweichel/provide-better-feedback-4828 branch July 19, 2021 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

provide better feedback if .gitpod.yml cannot be parsed
3 participants