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(sdk): Verify binary data in step copy-results-artifacts. Fixes #984 #985

Merged
merged 3 commits into from
Jul 6, 2022

Conversation

maxdebayser
Copy link
Contributor

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

Description of your changes:

Add a verification to copy-results-artifacts to prevent small binary files from being copied. The presence of unprintable characters in the PipelineRun status breaks the UpdateRun function in run_store.go

The verification with awk is compatible with the busybox container.

Environment tested:

  • Python Version (use python --version): 3.8
  • Tekton Version (use tkn version): 1.5
  • Kubernetes Version (use kubectl version): v1.23.5
  • OS (e.g. from /etc/os-release): Fedora 35

@google-cla
Copy link

google-cla bot commented Jul 4, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@Tomcli
Copy link
Member

Tomcli commented Jul 5, 2022

Hi @maxdebayser, can your run make unit_test GENERATE_GOLDEN_YAML=True with the new python sdk code to regenerate the unit test yaml? So it can pass our github action tests

@google-oss-prow google-oss-prow bot added size/M and removed size/XS labels Jul 5, 2022
@Tomcli
Copy link
Member

Tomcli commented Jul 5, 2022

/lgtm
@maxdebayser once you signed the cla, please let me know so I can retrigger the cla check.

@maxdebayser
Copy link
Contributor Author

@Tomcli , now when I visit https://cla.developers.google.com/ it says I'm covered by Google Corporate CLA. Is that all or do I have to do something more?

@Tomcli
Copy link
Member

Tomcli commented Jul 6, 2022

@maxdebayser based on https://github.com/kubeflow/kfp-tekton/pull/985/checks?check_run_id=7219006673
Seems like one of your commit is not using the email with signed CLA. Maybe you can squash your commits into one?

@maxdebayser
Copy link
Contributor Author

@Tomcli I've verified that my IBM e-mail address is set in my commits. I've also added my IBM e-mail as a secondary to may github.com account. And now I've also clicked on "New Contributors: Update this check after signing the CLA by clicking [here]" link in the CLA check message. Apparently it's passing now.

@Tomcli
Copy link
Member

Tomcli commented Jul 6, 2022

thanks @maxdebayser
/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maxdebayser, Tomcli

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

@google-oss-prow google-oss-prow bot merged commit bce9814 into kubeflow:master Jul 6, 2022
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.

Passing small binary files with OutputPath prevents KFP to update run status
2 participants