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

URLEncode instead of base64 encode the filter string #620

Merged
merged 1 commit into from
Jan 4, 2019

Conversation

neuromage
Copy link
Contributor

@neuromage neuromage commented Jan 3, 2019

This change is Reviewable

@neuromage
Copy link
Contributor Author

/assign @rileyjbauer
/cc @yebrahim
/cc @Ark-kun

@yebrahim
Copy link
Contributor

yebrahim commented Jan 3, 2019

Why is this better?

@neuromage
Copy link
Contributor Author

@yebrahim for some context, we discussed base64 vs urlencoding with @Ark-kun and @rileyjbauer yesterday and decided to switch here, mainly due to:

  • @rileyjbauer points out it's slightly easier when debugging to parse the string
  • @Ark-kun points out that it's probably what most http client libraries do when given a map of key value pairs and asked to construct a url query from them

@rileyjbauer
Copy link
Contributor

/lgtm

@yebrahim
Copy link
Contributor

yebrahim commented Jan 3, 2019

I see, sgtm then.

@neuromage
Copy link
Contributor Author

/assign @IronPan for approval

@k8s-ci-robot
Copy link
Contributor

@neuromage: GitHub didn't allow me to assign the following users: for, approval.

Note that only kubeflow members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign @IronPan for approval

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

1 similar comment
@k8s-ci-robot
Copy link
Contributor

@neuromage: GitHub didn't allow me to assign the following users: for, approval.

Note that only kubeflow members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign @IronPan for approval

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@IronPan
Copy link
Member

IronPan commented Jan 4, 2019

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

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

1 similar comment
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

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

@IronPan
Copy link
Member

IronPan commented Jan 4, 2019

Thanks @neuromage
@yebrahim url encoding is good for cases that human readable adds benefits which this case does.

@k8s-ci-robot k8s-ci-robot merged commit 65d0f6a into kubeflow:master Jan 4, 2019
Linchin pushed a commit to Linchin/pipelines that referenced this pull request Apr 11, 2023
* Add a README explaining how auto deploy works.

* Delete the old cron jobs since these are no longer used.

Related to kubeflow#595
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.

5 participants