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 unhandled error in actions_artifacts.go #2460

Merged
merged 6 commits into from Sep 15, 2022

Conversation

mo1ein
Copy link
Contributor

@mo1ein mo1ein commented Sep 13, 2022

No description provided.

@google-cla
Copy link

google-cla bot commented Sep 13, 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.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @mo1ein !
Could you please add a unit test that exercises this new code path?

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 13, 2022

It also looks like you need to run gofmt on the edited file to fix the formatting. Please see our CONTRIBUTING.md guide for more information.

@codecov
Copy link

codecov bot commented Sep 13, 2022

Codecov Report

Merging #2460 (1ac8032) into master (e2f7379) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2460   +/-   ##
=======================================
  Coverage   98.03%   98.03%           
=======================================
  Files         122      122           
  Lines       10689    10692    +3     
=======================================
+ Hits        10479    10482    +3     
  Misses        144      144           
  Partials       66       66           
Impacted Files Coverage Δ
github/actions_artifacts.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mo1ein
Copy link
Contributor Author

mo1ein commented Sep 13, 2022

It also looks like you need to run gofmt on the edited file to fix the formatting. Please see our CONTRIBUTING.md guide for more information.

Always after writing code, I run gofmt but for this file, I can't see 8 spaces after gofmt. That's the reason of my second commit.

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 13, 2022

It also looks like you need to run gofmt on the edited file to fix the formatting. Please see our CONTRIBUTING.md guide for more information.

Always after writing code, I run gofmt but for this file, I can't see 8 spaces after gofmt. That's the reason of my second commit.

gofmt uses tab characters and not spaces. Maybe that explains the difference?

@mo1ein
Copy link
Contributor Author

mo1ein commented Sep 13, 2022

It also looks like you need to run gofmt on the edited file to fix the formatting. Please see our CONTRIBUTING.md guide for more information.

Always after writing code, I run gofmt but for this file, I can't see 8 spaces after gofmt. That's the reason of my second commit.

gofmt uses tab characters and not spaces. Maybe that explains the difference?
OMG! yeah. Thank's for your clarifying.

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 13, 2022

yeah. Thank's for your clarifying.

Cool!

Could you please add a unit test that exercises this new code path?

@mo1ein
Copy link
Contributor Author

mo1ein commented Sep 13, 2022

yeah. Thank's for your clarifying.

Cool!

Could you please add a unit test that exercises this new code path?

I can't figure out how to fix Added lines #L125 - L126 were not covered by tests. Can you help?

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 13, 2022

I can't figure out how to fix Added lines #L125 - L126 were not covered by tests. Can you help?

I believe that if you give a "Location" value that url.Parse will return an error for, then that should test the line. Maybe something like "https://\n/bogus\nurl" or something like that?

@mo1ein
Copy link
Contributor Author

mo1ein commented Sep 13, 2022

I can't figure out how to fix Added lines #L125 - L126 were not covered by tests. Can you help?

I believe that if you give a "Location" value that url.Parse will return an error for, then that should test the line. Maybe something like "https://\n/bogus\nurl" or something like that?

I added the test but it has a bug. I think the test idea is correct. Just need to produce URL parse error. I believe your review is helpful to fix bugs.

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 14, 2022

I added the test but it has a bug. I think the test idea is correct. Just need to produce URL parse error. I believe your review is helpful to fix bugs.

Apparently, according to the source code for url.Parse, if you put control characters in your URL, this function should return an error:

https://cs.opensource.google/go/go/+/refs/tags/go1.19.1:src/net/url/url.go;l=503-505;drc=d36466fe2aaf51c4b38c3a2ea3164cb3a56b8059

@mo1ein
Copy link
Contributor Author

mo1ein commented Sep 15, 2022

  • Fixed. Everything is OK. :-)

github/actions_artifacts.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @mo1ein !
LGTM.
Merging.

@gmlewis gmlewis merged commit d611561 into google:master Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants