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

Can set booleans (--live-update and/or --debug) to a non-boolean value via workload.yaml #91

Open
2 tasks done
heyjcollins opened this issue May 2, 2022 · 3 comments
Labels

Comments

@heyjcollins
Copy link
Contributor

Please fill out the issue checklist below and provide ALL the requested information.

  • I reviewed open and closed Github issues that may be related to my problem.
  • I am reporting a bug that others will be able to reproduce.

Describe the bug

There are currently two boolean flags that can be set via command flags and/or workload.yaml:

  • --live-update
  • --debug
    The apps plugin validates the values passed via command flags today and will throw an error if a non-boolean value is provided (e.g. --debug foo will fail validation).

However, it's possible to set spec.params.debug or spec.params.live-update to a value of foo and submit the workload.yaml without error.

Expected behavior

apps plugin should throw an error if/when workload.yaml includes non-boolean values for boolean workload spec properties.

Steps to Reproduce

  1. create a workload using the following workload.yaml
    ---
    apiVersion: carto.run/v1alpha1
    kind: Workload
    metadata:
      labels:
        app.kubernetes.io/part-of: pc
        apps.tanzu.vmware.com/workload-type: web
      name: pc
      namespace: default
    spec:
      params:
      - name: live-update
        value: foo
      - name: debug
        value: bar
      source:
        git:
          ref:
            tag: tap-1.1
          url: https://github.com/sample-accelerators/spring-petclinic
    

Version (Apps plugin version, Version of K8s running on cluster)

Put the output of the following command

tanzu version && tanzu apps version
version: v0.11.2
buildDate: 2022-03-17
sha: 9f16f375
v0.5.1

Environment where the bug was observed (cloud, OS, etc)

Additional context & Relevant Debug Output (Logs, etc)

cc @shashwathi (Hey Shash - I ended up logging this bug since I didn't see it this morning).

@heyjcollins heyjcollins added bug Something isn't working needs-triage labels May 2, 2022
@heyjcollins heyjcollins added this to the 0.7.0 milestone May 2, 2022
@heyjcollins
Copy link
Contributor Author

heyjcollins commented May 2, 2022

Had some additional offline discussions with @scothis regarding this issue.
We're using the params as a vehicle to enable/disable debug/live-update.
It would be better to namespace these params if we're going to apply validation as described here.
Namespaced params examples:

  • debug --> apps.tanzu.vmware.com/debug
  • live-update --> apps.tanzu.vmware.com/live-update
  • annotations --> apps.tanzu.vmware.com/annotations

Any changes to the params must be coordinated with corresponding changes to Cartographer supply chain(s).
We should hold off on addressing this issue until/unless we've done that.

@heyjcollins
Copy link
Contributor Author

label blocked applied because we need to namespace the labels before we can reasonably apply this custom boolean validation.

@atmandhol atmandhol removed this from the 0.7.0 milestone May 9, 2022
@heyjcollins
Copy link
Contributor Author

Not migrated:
it's potentially blocked on changes to carto v1 (if we move to using tanzu namespaced versions of the live-update and debug params, eg. live-update --> apps.tanzu.vmware.com/live-update ).
Although this will add some polish and prevent user error, the value in relation to overall priorities on the plate is questionable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants
@atmandhol @heyjcollins and others