Skip to content
This repository has been archived by the owner on Jan 10, 2023. It is now read-only.

Remove Public Read header from presigned PUT #105

Merged
merged 4 commits into from
Jun 10, 2020

Conversation

hootener
Copy link
Contributor

@hootener hootener commented May 18, 2020

No description provided.

@hootener
Copy link
Contributor Author

This seems to be failing in CI with:

Restoring NuGet package Cake.0.32.1.
  GET https://api.nuget.org/v3-flatcontainer/cake/0.32.1/cake.0.32.1.nupkg
  OK https://api.nuget.org/v3-flatcontainer/cake/0.32.1/cake.0.32.1.nupkg 12ms
Installing Cake 0.32.1.
Adding package 'Cake.0.32.1' to folder '/home/appveyor/projects/codecov-exe/tools'
Added package 'Cake.0.32.1' to folder '/home/appveyor/projects/codecov-exe/tools'
Error: The assembly name is invalid.
Command exited with code 1
Build failed

And I just don't possess the knowledge of this application to make a fix here. Please let me know if I can help, @AdmiringWorm and @larzw

@AdmiringWorm
Copy link
Collaborator

@hootener thank you for opening this PR. I was not aware that the storage location for S3 had been changed to something else as thus not needing this request header anymore.

Regarding the CI error, this is something I have been struggling to figure out myself as well, but so far have been unsuccessful in finding out why the failures happen.
The build only fails on Unix, and I believe there may be an incompatibility with the mono version on the CI server compared to earlier versions.

I may need to rewrite the build scripts to before I can merge in this PR.

@AdmiringWorm AdmiringWorm added this to the 1.11.0 milestone May 22, 2020
@hootener
Copy link
Contributor Author

I may need to rewrite the build scripts to before I can merge in this PR.

@AdmiringWorm Got it. Please let me know if there's anything else I need to do with respect to this PR or to unblock this process. Thanks!

@sn1020
Copy link

sn1020 commented Jun 2, 2020

@hootener Might be helpful to add a readme update to this PR, similar to codecov/codecov-node#181?

@hootener
Copy link
Contributor Author

hootener commented Jun 2, 2020

Done @sn1020 .

@AdmiringWorm I see this CI on this is passing now. Is it good to merge and release version 1.11.0?

@AdmiringWorm
Copy link
Collaborator

@hootener Unfortunately not yet, still working on a fixed build script.
The check succeeded because only the GitHub action ran (which unfortunately do not do any integration test yet), not the appveyor one which is the main one (it doesn't run on pull requests unfortunately due to missing the pull request event hook).

So I need to be sure that it works before merging this one in.

@AdmiringWorm AdmiringWorm changed the base branch from master to develop June 9, 2020 04:12
@codecov codecov deleted a comment from codecov bot Jun 10, 2020
@codecov
Copy link

codecov bot commented Jun 10, 2020

Codecov Report

Merging #105 into develop will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #105   +/-   ##
========================================
  Coverage    66.94%   66.94%           
========================================
  Files           36       36           
  Lines          947      947           
  Branches       120      120           
========================================
  Hits           634      634           
  Misses         313      313           
Impacted Files Coverage Δ
Source/Codecov/Upload/CodecovUploader.cs 10.90% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89b457b...c0c41ba. Read the comment docs.

Copy link
Collaborator

@AdmiringWorm AdmiringWorm left a comment

Choose a reason for hiding this comment

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

LGTM

@AdmiringWorm AdmiringWorm merged commit d88ddc7 into develop Jun 10, 2020
@AdmiringWorm AdmiringWorm deleted the fix_public_read_header branch June 10, 2020 01:25
@AdmiringWorm
Copy link
Collaborator

@hootener your changes have been merged, thanks for your contribution 👍

@thomasrockhu
Copy link
Contributor

Thanks @AdmiringWorm!

@github-actions
Copy link

🎉 This issue has been resolved in version 1.11.0 🎉

The release is available on:

Your friendly GitReleaseManager bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants