Create/Get/Update/Delete references #97
Conversation
raghav710
commented
Mar 24, 2019
raghav710
commented
- Tests for a few common scenarios
- Only Get and Create references implemented as of now as it is required for Repositories - Merging #84
- PSScriptAnalyser Run returns no issues
|
I'm planning to rebase my #84 branch on top of this so I can get that ready for review as well while this is being reviewed |
|
@HowardWolosky dropping a note to update that I'll take a look at this probably this weekend. (I wish I could spend more time on this repo) |
|
Thanks for the update. No worries. I'll keep this PR open as long as is needed. I appreciate the contribution. |
|
@HowardWolosky no need to review this yet, will make a few more changes and address remaining comments before its ready for review again. Thanks for being patient, happy to be back! |
raghav710
force-pushed
the
issue-#96-gitrefs-implement
branch
from
7a564c7 to
4937cdb
Compare
raghav710
force-pushed
the
issue-#96-gitrefs-implement
branch
from
4937cdb to
3a359af
Compare
|
@HowardWolosky this is now ready for another round of review. I havent added the Update and delete bits yet and plan to do so once I'm done with my other Pull request, so this is kind of minimal. Also reduces review effort as a result :) |
There was a problem hiding this comment.
Thanks for the update (and sorry for the delay -- I missed that you had submitted an update to this such a long time ago).
There are some additional changes being requested here, but they're pretty minor.
I know that you said you were only planning on adding Get and Create as part of the PR, but any change you're interested in adding Update(https://developer.github.com/v3/git/refs/#update-a-reference) and Delete(https://developer.github.com/v3/git/refs/#delete-a-reference) as well, given that they're so closely tied together, especially from a testing perspective?
Additionally, it looks like you missed how the matching references works (or I'm misunderstanding your intent with the Get).
Thanks again!
|
@HowardWolosky thanks for the comments. I'll make the required changes and submit for review |
raghav710
changed the title
raghav710
force-pushed
the
issue-#96-gitrefs-implement
branch
from
d994230 to
2a1be73
Compare
|
@HowardWolosky I've incorporated your comments and added the update and delete methods. This is ready for another round of review
Figured it out. Was giving the ref name wrongly. Will add tests and update the PR Also, I rebased the changes on top of master and later pulled from this branch which caused some issues (started showing 26 files as changed), hence I squashed the commits and after ensuring it is rebased on top of latest master, have force pushed it. Sorry if this makes the review difficult |
raghav710
force-pushed
the
issue-#96-gitrefs-implement
branch
from
2a1be73 to
aa51945
Compare
raghav710
force-pushed
the
issue-#96-gitrefs-implement
branch
from
aa51945 to
c56a2d0
Compare
|
@HowardWolosky these tests require the Contents API to be implemented. Have created an issue for that and will be working on it UPDATE: |
raghav710
force-pushed
the
issue-#96-gitrefs-implement
branch
2 times, most recently
from
07c17c6 to
869d52c
Compare
This commit adds support for the create/update/delete and Get reference APIs. At this point the test cases for Update alone need to be added
|
@HowardWolosky this is now ready for review. Please let me know your comments on this. |
There was a problem hiding this comment.
Hi there,
Thanks so much for revisiting this and providing some updates. I've added a bunch of feedback that I'd ask you to look at and try to apply. Happy to provide more context if needed.
Given the changes being requested in the main code, I skipped reviewing the test code for now. I'll review that again once an update comes in addressing the core code.
Again, thanks so much for your continued interest and contributions!
HowardWolosky
added
the
api completeness
raghav710
force-pushed
the
issue-#96-gitrefs-implement
branch
5 times, most recently
from
0334cca to
c4369d7
Compare
raghav710
force-pushed
the
issue-#96-gitrefs-implement
branch
from
c4369d7 to
fa1194e
Compare
|
@HowardWolosky this is now ready for another round of review. Sorry there are multiple commits, I would like to squash these before merging. Please let me know your thoughts on the Get-GithubReference, I had to take a few decisions with the parameters to accomodate the different use cases. Have also updated the tests to be more extensive. |
|
@HowardWolosky please feel free to comment on the approach used here and whether it is good enough. Also if you think some of this functionality is already being added by #200 I'm fine with removing parts of these changes so we can bring in only what is required |
There was a problem hiding this comment.
Thanks for all of this!
You're definitely on the right track.
There are a bunch of nits to address and a few fundamental changes. There's also the need to get pipeline support added to these functions as well.
My expectation is that we'll move forward with merging in your changes once everything has been addressed, and then we'll have #200 build on top of this functionality (so, New-GitHubRespositoryBranch would just internally call into New-GitHubReference when creating the reference).
|
Thanks for the comments @HowardWolosky . I would take some time in getting back to this. Is there a timeline we are looking at to get this merged? (So the dependent PRs dont have to wait?) and I can try working with that in mind |
Do you think you can get to it within the next two weeks? If not, then maybe someone else in the community might be able to apply the feedback to what you've done so far, in order to unblock the other PR's waiting on this (#200 is definitely waiting on this, but there are others like #193 which need it for their UT's). You've put a bunch of effort into this so far, and I want to ensure that you're getting the appropriate credit for your work. I also want to keep the momentum going for the others that are also actively contributing to the project and prevent blockages from happening for too long. Thanks for your understanding and your participation! |
HowardWolosky
added
the
waiting for update
|
@HowardWolosky I plan to work on this today and fix the code based on comments |
This change adds pipeline support for the GitHubReference module and in addition * Fixes naming of variables and methods * Adds comment based help for all methods * Removes the temporary method used in testing for updating a branch and uses Set-Content instead * Updates test case to add pipeline scenarios * Verified by running test cases and ScriptAnalyzer
raghav710
force-pushed
the
issue-#96-gitrefs-implement
branch
from
ba87857 to
458e6dd
Compare
|
@HowardWolosky I have made an attempt at adding pipeline support and refactored where required to match the current standards. Also updated the tests to check for pipelines and to follow Pester standards where applicable. Taking this forward, would someone in the community be willing to take this from here? Given that there are other PRs waiting on this, I dont want to delay this any longer waiting for my availability. PS: its amazing how much has been done in the project in a short period! from pipeline support to removing boilerplate from tests to standardizing many code conventions. Kudos to you and the community! |
|
Thanks for all your efforts here, @raghav710. You've provided a solid base for the remaining work that needs to be done. I'm nearing completion on an update to this work to fill in the remaining gaps. Once I'm done with that, I'll close out this PR and start a new one. Again, thanks so much. Looking forward to seeing your further contributions in this project when you have time. |
|
Thanks @HowardWolosky for giving me a chance to contribute to this project. Again, apologies for not taking this to completion |
