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 for backtick escape error #358

Merged
merged 2 commits into from
Sep 16, 2019
Merged

Conversation

kkasravi
Copy link
Contributor

@kkasravi kkasravi commented Sep 13, 2019

Which issue is resolved by this Pull Request:
Resolves #356

Description of your changes:
backticks are escaped as described in the issue.
a second fix is related to returning the earliest 'manifests' in a directory path rather than the latest. If a subdir ends with manifests you will get an error in unit test generation.

Checklist:

  • Unit tests have been rebuilt:
    1. cd manifests/tests
    2. make generate
    3. make test

This change is Reviewable

@kkasravi
Copy link
Contributor Author

/assign @jlewi
/assign @gabrielwen
/assign @swiftdiaries

@kkasravi
Copy link
Contributor Author

/assign @hougangliu

@jlewi
Copy link
Contributor

jlewi commented Sep 15, 2019

@kkasravi Why use escaping at all? i.e. rather than embed YAML in go files; why not just create YAML files and have the unittests load those files? If this is a quick fix then doing that then I think its fine.

Longer term though it seems like #306 (verifying expected values) is pushing us in the direction of creating a test_data directory that would contain YAML files representing the expected output of kustomize build.

So storing YAML files as opposed to embedding YAML in go files seems more consistent with that.

That said, there's probably a lot more changes needed to realize #306 and I'm not sure that changing the current scripts to emit the kustomize files to YAML rather than embedding in the go code really moves the needle. So I'm fine with the workaround in this PR.

@hougangliu
Copy link
Member

please resolve the conflict. lgtm

@kkasravi
Copy link
Contributor Author

thanks @jlewi - it would be more straight forward to store the files in a different directory and read them in.

@jlewi
Copy link
Contributor

jlewi commented Sep 16, 2019

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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

@k8s-ci-robot k8s-ci-robot merged commit d47d06a into kubeflow:master Sep 16, 2019
@kkasravi kkasravi deleted the backtick-fix branch October 4, 2019 18:43
Tomcli pushed a commit to Tomcli/manifests that referenced this pull request Dec 11, 2020
* Quick fix in SDK documentation

Takes some excerpts from Christian's website PR. 

I will be revisiting after release to figure out the overlap with User guide.

* SDK Packages Overview (kubeflow#359)

Co-authored-by: Christian Kadner <ckadner@us.ibm.com>
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.

unit test generation isn't properly escaping embedded backticks
7 participants