-
Notifications
You must be signed in to change notification settings - Fork 188
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
Support for Set-GitHubContent to Upload binary files #364
Support for Set-GitHubContent to Upload binary files #364
Conversation
6a2f707
to
c839c57
Compare
c839c57
to
83beb95
Compare
Thanks @StanleyGoldman -- I have this queued for review this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting close -- thanks for reviving this change. Looking forward to getting it merged in.
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 14 days of this comment. |
This pull request has been automatically closed due to a lack of activity from the author. We understand. Life happens and other things likely came up. We would still love to see your contribution get merged in. Now that it has been closed, a different community member may wish to pick up where you left off. If so, they should speak up by commenting below. If you're still interested in completing this yourself, just respond back and let us know. |
Heya, I'd still love to get this change in. |
That's great news. I'm re-opening the PR. Your next step is to address the PR feedback and to resolve the merge conflicts. |
Co-authored-by: Howard Wolosky <[email protected]>
Co-authored-by: Howard Wolosky <[email protected]>
Co-authored-by: Howard Wolosky <[email protected]>
@HowardWolosky ready when you are. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update.
Looks like you still have a few more changes to make, primarily in the tests (were those successfully running locally for you?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small bit of a additional feedback. Thanks for the effort here, and apologies for delay in review.
Co-authored-by: Howard Wolosky <[email protected]>
/azp run PowerShellForGitHub-CI |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run PowerShellForGitHub-CI |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run PowerShellForGitHub-CI |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run PowerShellForGitHub-CI |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for taking so long to finally review this update.
The product change looks good.
On the test side, the first test is also good.
The only concern that I have is that it looks like you intended for your second test to be dependent on the first test (in that you're hoping to validate that you can overwrite existing content that was written by the previous test. The issue of that is that each of these Contexts should be able to run independently. If you want a context that is testing overwriting existing content, then you should have that happen within the scope of a single Context.
Ideally we should be able to repro a test bug by running just the failing test, which wouldn't be possible if the failure only repro'd if something needed to happen in a separate context first.
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 14 days of this comment. |
1 similar comment
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 14 days of this comment. |
This pull request has been automatically closed due to a lack of activity from the author. We understand. Life happens and other things likely came up. We would still love to see your contribution get merged in. Now that it has been closed, a different community member may wish to pick up where you left off. If so, they should speak up by commenting below. If you're still interested in completing this yourself, just respond back and let us know. |
Description
As mentioned in #335, this pull request adds functionality to upload binary content with Set-GitHubContent
#336 got closed to due inactivity. Apologies. I'm still very interested in getting this code merged.
Issues Fixed
Fixes #335
References
Checklist