Fix edge cases such as disable istio-inject for Tekton compiler #119
Conversation
|
This change is |
| @@ -236,6 +237,23 @@ def _test_pipeline_workflow(self, | |||
| compiled = list(yaml.safe_load_all(f)) | |||
| if GENERATE_GOLDEN_YAML: | |||
| with open(golden_yaml_file, 'w') as f: | |||
| f.write(textwrap.dedent("""\ | |||
There was a problem hiding this comment.
It's a good idea to add the license header! Can you move this text into a variable at the top of the file
| @@ -36,18 +36,6 @@ | |||
| from .. import tekton_api_version | |||
|
|
|||
|
|
|||
| def _literal_str_representer(dumper, data): | |||
There was a problem hiding this comment.
I forgot the history of this, can you quickly explain why we had to add this and why it's no longer needed?
There was a problem hiding this comment.
I wanted to add literal style to make the script part looks nicer. But there's some edge cases with pyyaml when using it on integer since it's designed with yaml 1.1
Here's the definition of literal style in yaml
https://yaml.org/spec/1.2/spec.html#id2795688
| 'name': 'copy-results', | ||
| 'script': '#!/bin/sh\nset -exo pipefail\n' | ||
| } | ||
| volume_mount_step_template = [] |
There was a problem hiding this comment.
aren't those still being used in the code below?
| # initial base_step and volume setup | ||
| base_step = { | ||
| 'image': 'busybox', | ||
| 'name': 'copy-results', |
There was a problem hiding this comment.
I still see the copy-results step in the compiled YAML and in a comment, does that come from somewhere else?
There was a problem hiding this comment.
I moved this into a different function, but somehow it get put back after the merge. We no longer need this over here because it is initiated at the beginning of this function.
| if GENERATE_GOLDEN_YAML: | ||
| with open(golden_yaml_file, 'w') as f: | ||
| f.write(textwrap.dedent("""\ | ||
| license_header = textwrap.dedent("""\ |
There was a problem hiding this comment.
I meant move license_header to the top of the compiler_tests.py file :-) ... since Andrew will have to use that in his PR as well and it blows up the code too much here
There was a problem hiding this comment.
oh you mean set it as global variable?
| @@ -27,6 +27,24 @@ | |||
| # in order to generate new "golden" YAML files | |||
| GENERATE_GOLDEN_YAML = False | |||
|
|
|||
| # License header for Kubeflow project | |||
| LICENSE_HEADER = textwrap.dedent("""\ | |||
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ckadner 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 |
* Add unit tests * Update condition.yaml * Update test_kfp_samples.sh for new test_util.py * Add results logging and refactor * Refactor and add licenses * Add license * Add __main__ to introduced testdata * Adjust new testdata for #119 * Add and edit doc string * Fix golden yaml
…error Bugfix: Benign modified DSPA object error
Which issue is resolved by this Pull Request:
Resolves #114
Description of your changes:
Environment tested:
python --version): 3.6.4tkn version): 0.11.3kubectl version):/etc/os-release):