allow GetRequirementsConfigFromTeamSettings to be less restrictive validating settings

romainverduci commented 4 years ago

Summary

In case there is a change in the team settings, we want some tolerance in the validation to not fail when there is a new field which does not impact existing ones for example.

This has been done in https://github.com/jenkins-x/jx/pull/7249 already so we just want to extand this behaviour in the GetRequirementsConfigFromTeamSettings method.

Concrete issue happening:

There was a field added in the team settings in this PR: https://github.com/jenkins-x/jx/pull/7172
It made import / quickstart failing because of:
```
error getting Requirements from TeamSettings to determine if in GitHub app mode: validation failuresi in requirements from team settings: (root): additional property buildPacks is not allowed
```
It looks like the validation failure comes from this method: https://github.com/jenkins-x/jx/blob/7ae7aac6e2af35b30cbe02193871f61d8169f835/pkg/config/install_requirements.go#L629

Adding a field in team settings should not fail this validation.

hferentschik commented 4 years ago

@romainverduci, can you provide the jx-requirements.yml as stored in the team settings? I would, in particular, be interested to know whether there was a buildPacks entry.

Which version of jx were you using at that stage?

romainverduci commented 4 years ago

Here is the jx-requirements.yml from the dev env repo:

autoUpdate:
  enabled: true
  schedule: 0 0 * * *
bootConfigURL: https://github.com/cloudbees/arcalos-boot-config.git
buildPacks:
  buildPackLibrary: {}
cluster:
  azure: {}
  chartRepository: http://bucketrepo/bucketrepo/charts/
  clusterName: elfperidot
  environmentGitOwner: core-platform-test-org
  environmentGitPublic: true
  externalDNSSAName: elfperidot-dn
  gitKind: github
  gitName: github
  gitPublic: true
  gitServer: https://github.com
  gke:
    projectNumber: "893173526554"
  kanikoSAName: elfperidot-ko
  namespace: jx
  project: cbjx-elfperidot
  provider: gke
  registry: gcr.io
  vaultName: elfperidot
  vaultSAName: elfperidot-vt
  zone: us-central1-c
environments:
- ingress:
    cloud_dns_secret_name: external-dns-gcp-sa
    domain: cbjx-elfperidot.play-jxaas.live
    domainIssuerURL: https://jx-tenant-service-jx.mgmt.play-jxaas.live
    externalDNS: true
    namespaceSubDomain: -jx.
    tls:
      email: jenkins-x@cloudbees.com
      enabled: true
      production: false
  key: dev
  repository: environment-elfperidot-dev
- ingress:
    domain: ""
    externalDNS: false
    namespaceSubDomain: ""
    tls:
      email: ""
      enabled: false
      production: false
  key: staging
  repository: environment-elfperidot-staging
- ingress:
    domain: ""
    externalDNS: false
    namespaceSubDomain: ""
    tls:
      email: ""
      enabled: false
      production: false
  key: production
  repository: environment-elfperidot-production
githubApp:
  enabled: true
  schedule: 0 0 * * *
  url: https://lighthouse-githubapp-jx.mgmt.play-jxaas.live
gitops: true
ingress:
  cloud_dns_secret_name: external-dns-gcp-sa
  domain: cbjx-elfperidot.play-jxaas.live
  domainIssuerURL: https://jx-tenant-service-jx.mgmt.play-jxaas.live
  externalDNS: true
  namespaceSubDomain: -jx.
  tls:
    email: jenkins-x@cloudbees.com
    enabled: true
    production: false
kaniko: true
repository: bucketrepo
secretStorage: vault
storage:
  backup:
    enabled: true
    url: gs://cbjx-elfperidot-backup
  logs:
    enabled: true
    url: gs://cbjx-elfperidot-lts
  reports:
    enabled: false
    url: ""
  repository:
    enabled: true
    url: gs://cbjx-elfperidot-repository
vault:
  aws: {}
  bucket: cbjx-elfperidot-vault
  key: elfperidot-52a3c7bf51e0-crypto-key
  keyring: elfperidot-52a3c7bf51e0-keyring
  name: elfperidot
  serviceAccount: elfperidot-vt
velero:
  namespace: velero
  schedule: ""
  serviceAccount: elfperidot-vo
  ttl: ""
versionStream:
  ref: v0.0.466
  url: https://github.com/cloudbees/arcalos-jenkins-x-versions.git
webhook: lighthouse

So there is a buildPack entry. The version of jx used to create the quickstart is v2.1.42 which does not contain the new field to validate so that's why the validation is failing.

hferentschik commented 4 years ago

Thanks a lot. So here is what I don't understand:

buildPacks:
  buildPackLibrary: {}

How did this get into the requirements you are trying to parse? With this piece of config in the requirements trying I understand how an older version of jx will fail validation. However, the whole idea was that unless you are not using this new feature you won't get this entry. We even caught one obvious issue where BuildPackConfig was added to RequirementsConfig as an instance not a pointer. Now BuildPackConfig is a pointer https://github.com/jenkins-x/jx/blob/7ae7aac6e2af35b30cbe02193871f61d8169f835/pkg/config/install_requirements.go#L522 with the intention that nothing is added to the serialized YAML unless you are using this new functionality. There is even a test which tries to make sure that this config is not "accidentally" added when we serialize the default struct. Somewhere something is not working as expected and that is what worries me. Whether we should relax the validation or maybe make validation a separate function altogether is a valid question, but before we do so I'd like to understand how this happens since it was for sure not intended this way.

hferentschik commented 4 years ago

So instead of further adding more flags to ignore validation errors, I suggest to change:

func LoadRequirementsConfig(dir string, failOnValidationErrors bool) (*RequirementsConfig, string, error)

to

func LoadRequirementsConfig(dir string) (*RequirementsConfig, string, error)
func ValidateYaml(requirementsFile string) error

Calling the validation becomes a choice in this case.

There is also potentially a bug in step_verify_requirements.go where the wrong YAML library is used to seralize/deserialilze the requirements, leading to fields which were meant to be empty to get initialized, for example:

buildPacks:
  buildPackLibrary: {}

garethjevans commented 4 years ago

We would also need a function to LoadRequirementsConfigFromTeamSettings (without validation) as the file doesn't exist

jenkins-x-bot commented 4 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with /close. Provide feedback via https://jenkins-x.io/community. /lifecycle stale

jenkins-x-bot commented 4 years ago

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity. If this issue is safe to close now please do so with /close. Provide feedback via https://jenkins-x.io/community. /lifecycle rotten

jenkins-x-bot commented 4 years ago

Rotten issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle rotten. Provide feedback via https://jenkins-x.io/community. /close

jenkins-x-bot commented 4 years ago

@jenkins-x-bot: Closing this issue.

In response to [this](https://github.com/jenkins-x/jx/issues/7288#issuecomment-724033701): >Rotten issues close after 30d of inactivity. >Reopen the issue with `/reopen`. >Mark the issue as fresh with `/remove-lifecycle rotten`. >Provide feedback via https://jenkins-x.io/community. >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [jenkins-x/lighthouse](https://github.com/jenkins-x/lighthouse/issues/new?title=Command%20issue:) repository.

jenkins-x / jx

allow GetRequirementsConfigFromTeamSettings to be less restrictive validating settings #7288

Summary