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: update document against target format check and add hints #5361

Merged
merged 2 commits into from
Dec 4, 2024

Conversation

waynexia
Copy link
Member

Which issue does this PR close?

Closes #.

Rationale for this change

The document about oli's usage is misleading

What changes are included in this PR?

  • Update document
  • Add hint to error text
  • Add expected format to help text

Are there any user-facing changes?

Yes, some text

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
fleet.png
update-ecs-loadbalancer.json
```

### Example: use `oli` copy file from S3 to R2

```text
$ oli cp s3://fleet.png r2://fleet.png
$ oli ls r2://
$ oli cp s3:/fleet.png r2:/fleet.png
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's more like a bug to me 😭

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I convinced myself this is by design 🙈

@@ -147,7 +147,7 @@ impl Config {

let location = Url::parse(s)?;
if location.has_host() {
Err(anyhow!("Host part in a location is not supported."))?;
Err(anyhow!("Host part in a location is not supported. Hint: are you typing `://` instead of `:/`?"))?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems we are parsing the url in wrong.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior is not what I designed, but I agree that we should update the documentation to make it work first. We can discuss the implementation later.

@Xuanwo Xuanwo merged commit 6306acf into apache:main Dec 4, 2024
12 checks passed
@waynexia waynexia deleted the refine-oli-target branch December 4, 2024 08:02
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.

2 participants