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

Add dataBase64 flag for Ping Source #1204

Closed
xtreme-sameer-vohra opened this issue Jan 26, 2021 · 9 comments · Fixed by #1388 or #1392
Closed

Add dataBase64 flag for Ping Source #1204

xtreme-sameer-vohra opened this issue Jan 26, 2021 · 9 comments · Fixed by #1388 or #1392
Assignees
Labels
good first issue Denotes an issue ready for a new contributor. kind/feature New feature or request triage/accepted Issues which should be fixed (post-triage)

Comments

@xtreme-sameer-vohra
Copy link
Contributor

Feature request

For parity to be able to create a ping source using the kn cli

Use case

Use the dataBase64 for binary data as shown in the docs

kn source ping create test-ping-source --schedule "*/2 * * * *" --dataBase64: "ZGF0YQ==" --sink ksvc:event-display

@dsimansk
Copy link
Contributor

I wonder if should just add a plain --dataBase64 flag, that's pretty much straightforward and fast to do.
On the other hand I'd like something like more sophisticated --data flag that could try to detect base64 encoding automatically, or --data "base64:Hello!" that encodes the data for the kn user.

Wdyt? @rhuss @navidshaikh @maximilien

@dsimansk
Copy link
Contributor

dsimansk commented Feb 1, 2021

In addition a quick observation dataBase64 is available sources/v1beta2, but kn is still on v1alpha2 for the backward compatibility support.

https://github.com/knative/eventing/blob/master/pkg/apis/sources/v1beta2/ping_types.go#L80-L83
https://github.com/knative/client/blob/master/pkg/sources/v1alpha2/ping_client.go#L24

@eclipselu
Copy link

ContentType is also needed if we consider feature parity with v1beta2 PingSource.

@github-actions
Copy link

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 30, 2021
@dsimansk
Copy link
Contributor

dsimansk commented Jul 1, 2021

/remove-lifecycle stale
/kind good-first-issue

@knative-prow-robot knative-prow-robot added good first issue Denotes an issue ready for a new contributor. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 1, 2021
@rhuss rhuss added the triage/accepted Issues which should be fixed (post-triage) label Jul 6, 2021
@dsimansk
Copy link
Contributor

/assign @vyasgun

@knative-prow-robot
Copy link
Contributor

@dsimansk: GitHub didn't allow me to assign the following users: vyasgun.

Note that only knative members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @vyasgun

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.

@vyasgun
Copy link
Contributor

vyasgun commented Jul 12, 2021

/assign

@rhuss
Copy link
Contributor

rhuss commented Jul 13, 2021

I think we could have the following options:

  • --data : for string data (we have that)
  • --encoding=text or base64 to specify the encoding (i.e. which field to use and how to deal with the encoding). When base64 is given, encode the data (if not already encoded maybe) and use the base64 data field in the resource
  • --file for specifying a file. I think this is important as for binary data you typically want to pick it up from a file, encode it and send it. A file would be base64 encoded by default, only when --encoding text is given, then use the file literally and put it into data: field.

Actually, I would not offer an option to specify literal bas64 on the CLI but would do the translation all internally (and also wouldn't expose the field to be set directly). That is the value-add that a CLI can offer.
However we could be smart and detect whether a given --data is already a valid base64 encoded. If this is the case and no --encoding is given, then autodetect the field to put the data on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor. kind/feature New feature or request triage/accepted Issues which should be fixed (post-triage)
Projects
None yet
6 participants