-
Notifications
You must be signed in to change notification settings - Fork 122
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
feat(sdk): added sanitizing k8s object process and test. Fixes: #332 #345
feat(sdk): added sanitizing k8s object process and test. Fixes: #332 #345
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@Tomcli How to sign CLA? Shall I choose |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Hi @jizg there is a process that you need to follow to join the IBM Google CLA group before you are covered under the CLA. I have sent you a slack message with the IBM internal link to the instructions. |
Thanks @jizg for your contribution. This PR looks good to me. We can approve this PR once you signed the cla. |
I have raised a request to join IBM contributor google group, and it should be soon to sign the CLA. |
@googlebot I signed it! |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
@Tomcli I've signed the CLA. Would you please check whether it's ok to merge this PR? Thanks |
thanks @jizg |
|
||
|
||
def _to_str(s): | ||
return None if s is None else str(s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jizg, quick question. Is there a reason we need to check for None
here? Doesn't this get checked in sanitize_k8s_object? Or are there plans to use _to_str ()
outside this file in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, '_to_str()' is not necessary now. I firstly did the 'None' type check in '_to_str()', and then moved the check to sanitize_k8s_object
but forgot to remove '_to_str()'. I have updated the codes. Thanks @drewbutlerbb4 .
Thanks @jizg |
thanks @jizg |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jizg, 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 |
…sanitizing-k8s-objects * commit '3d3ecfb05fc13f400d4aec5ee5a0ca9b13b1566f': feat(sdk): added sanitizing k8s object process and test. Fixes: kubeflow#332 (kubeflow#345) add a warning label to kustomize page (kubeflow#350) make user guide prominent (kubeflow#349) feat(backend): ARTIFACT_COPY_STEP_TEMPLATE - custom template for copy step (kubeflow#336) Add KFP Tekton release deployment yaml (kubeflow#347) Fix UI merge conflict that blocks the UI image build. (kubeflow#346) KFP 1.0.4 Rebase (kubeflow#337) fix(sdk): Quick fix for sanitizing conditional variables (kubeflow#344) fix(backend): cover nil status race condition from Tekton (kubeflow#329) updated titanic-ml generated with kale 0.5.1 (kubeflow#330) Add s3 endpoint instructions for admin use case (kubeflow#323) Add best practice for tekton and other argo executors (kubeflow#338)
Which issue is resolved by this Pull Request:
Resolves #332
Description of your changes:
Added process and testcases for recursively sanitizing k8s container object type based on the kubernetes.client.models definition. Currently primitive types of string, integer, boolean, list of string are all the types included in kubernetes.client.models.V1Container and its children attributes.
Environment tested:
python --version
): 3.7.7tkn version
): 0.13.1kubectl version
): v1.16.15/etc/os-release
): Ubuntu 18.04Checklist:
The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.
PR titles examples:
fix(frontend): fixes empty page. Fixes #1234
Use
fix
to indicate that this PR fixes a bug.feat(backend): configurable service account. Fixes #1234, fixes #1235
Use
feat
to indicate that this PR adds a new feature.chore: set up changelog generation tools
Use
chore
to indicate that this PR makes some changes that users don't need to know.test: fix CI failure. Part of #1234
Use
part of
to indicate that a PR is working on an issue, but shouldn't close the issue when merged.Do you want this pull request (PR) cherry-picked into the current release branch?
If yes, use one of the following options:
cherrypick-approved
label to this PR. The release manager adds this PR to the release branch in a batch update.