microsoft
Create/Get/Update/Delete references
- Tests for a few common scenarios
- Only Get and Create references implemented as of now as it is required for #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 being patient!
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!
@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 :)
@HowardWolosky thanks for the comments. I'll make the required changes and submit for review
@HowardWolosky I've incorporated your comments and added the update and delete methods. This is ready for another round of review ~~However I wasn't able to test the update method's positive scenario and would appreciate any help on the same.~~
~~I tried creating a couple of branches in a sample repo (https://github.com/raghav710/61603191-d09d-4a1a-8a52-88e719371d8c/pulls) and tried to update the SHA of one of the branches to that of the other. But that didnt work.~~
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
@HowardWolosky these tests require the Contents API to be implemented. Have created an issue for that and will be working on it
UPDATE: Now that Get-Contents is available, will make use of the same for writing the tests
@HowardWolosky this is now ready for review. Please let me know your comments on this.
Thanks for the comments @HowardWolosky I'll take a look at the changes requested and try to get back as soon as possible. Sorry it has taken a year for this since I wasn't continuously involved
@raghav710 -- can you confirm that this is ready for re-review?
Additionally, I used GitHub's merge conflict resolver for you on the manifest file, and it appears that it messed up the line breaks in the file, causing the whole file to appear to have been changed. Can you please push two new commits, one where you revert the file to its original state, and the second where you re-apply your specific changes, so that only the changes are seen in the diff? Thanks!
@HowardWolosky sure. I'll do that. I checked the tests and i remember them running fine. Will check the code once again for any mistakes and planning to drop a note max by tomorrow that this is ready for review. Sorry this has taken so long
@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
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
Thanks for the comments @HowardWolosky Howard Wolosky FTE . 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 I plan to work on this today and fix the code based on comments
@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