Implement hands on hp-populate-remote

[oadultradeepfield]

Exercise Discussion

This PR will close #56. Below is a more detailed description of the changes along with implementation notes:

1. Added an add_origin function in exercise_utils/git.py. This might be helpful in the future. I noticed that there is already a remove_remote implementation, and add_origin is intended to be used in a similar way.
2. To help ensure that the remote repository gitmastery-things is created successfully across multiple attempts, I added a check to delete any existing remote repository with the same name. Please note that this requires setting up delete access for the GitHub CLI via gh auth refresh -h github.com -s delete_repo. I also realised that the GitHub CLI needs to be configured beforehand for this part of the hands-on to work.

Note: The download script tested via test-download.sh works fine. Here is the remote repo I managed to create.

Checklist

• If you require a new remote repository on the Git-Mastery organization, have you created a request for it?
• Have you written unit tests using repo-smith to validate the exercise grading scheme?
• Have you tested the download script using test-download.sh?
• Have you verified that this exercise does not already exist or is not currently in review?
• Did you introduce a new grading mechanism that should belong to git-autograder?
• Did you introduce a new dependency that should belong to app?

[woojiahao]

This isn't an easy hands-on to implement because we have remote integration. Left a couple of comments on structuring the hands-on!

Please note that this requires setting up delete access for the GitHub CLI via gh auth refresh -h github.com -s delete_repo.

@damithc are we able to include this in our setup guide if it's not yet included?

I also realised that the GitHub CLI needs to be configured beforehand for this part of the hands-on to work.

This is already supported when you do __require_github__ = True: https://git-mastery.github.io/developers/docs/architecture/hands-on-structure/#file-structure

[oadultradeepfield]

Thanks for the super-detailed comments! I’ll make the corresponding changes later today.

This is already supported when you do require_github = True: https://git-mastery.github.io/developers/docs/architecture/hands-on-structure/#file-structure

Sorry for the confusion, what I meant was that students need to have the GitHub CLI installed for the gh commands to work. I think it’s just an issue on my device since I haven’t used it before 😅.

[damithc]

Please note that this requires setting up delete access for the GitHub CLI via gh auth refresh -h github.com -s delete_repo.

@damithc are we able to include this in our setup guide if it's not yet included?

Given having to delete a remote repo is rare, instead of deleting the repo automatically, a safer alternative is to abort with a message asking the student to delete the repo and run the download operation again after doing so.
WDYT?

[oadultradeepfield]

Given having to delete a remote repo is rare, instead of deleting the repo automatically, a safer alternative is to abort with a message asking the student to delete the repo and run the download operation again after doing so. WDYT?

I agree with this approach! If I understand correctly, this would mean the Python script in this repo should raise exceptions that the app’s download.py can then catch and print user-friendly messages, right?

[woojiahao]

@damithc @oadultradeepfield afaik, deleting a repository is actually also used when we run gitmastery progress sync off. So we might need this piece of instruction regardless.

I think that it's worth

(1) Figuring out if we need to include delete_repo permission on a fresh install of gh (seems like it?)
(2) Either (a) adding a prompt to setup the delete_repo permission when we don't detect it or (b) Stopping and prompting users to delete it after.

I think (2) is a better user experience. What do you guys think?

[woojiahao]

Thank you for the quick turnaround @oadultradeepfield . I left some extra comments, but we will need to figure out on what to do with the delete first

[damithc]

(1) Figuring out if we need to include delete_repo permission on a fresh install of gh (seems like it?) (2) Either (a) adding a prompt to setup the delete_repo permission when we don't detect it or (b) Stopping and prompting users to delete it after.

I think (2) is a better user experience. What do you guys think?

@oadultradeepfield @woojiahao I'm OK with 2(b), given deleting repos is rare.

[oadultradeepfield]

@woojiahao @damithc Given that we’re likely proceeding with 2(b), may I confirm if the correct implementation is to raise an exception in this hands-on Python script? So far, I haven’t seen any hands-on printing any messages (though I might have missed it), so my understanding is that another part of the codebase (or a different repository) handles that.

[damithc]

@woojiahao @damithc Given that we’re likely proceeding with 2(b), may I confirm if the correct implementation is to raise an exception in this hands-on Python script? So far, I haven’t seen any hands-on printing any messages (though I might have missed it), so my understanding is that another part of the codebase (or a different repository) handles that.

@oadultradeepfield let's wait for @woojiahao's input on this.

[woojiahao]

@damithc @oadultradeepfield so sorry, resuming reviewing the PRs today.

I am okay with 2(b) as well. The caveat here is that you need to ensure that when the user runs gitmastery progress sync off, they should see this prompt to delete the progress fork. The good thing is that the app already defines deleting repositories as a central function: https://github.com/git-mastery/app/blob/main/app/utils/gh_cli.py#L83-L84. This would involve a separate PR down the line, but I am okay with tracking this as a follow-up issue.

if the correct implementation is to raise an exception in this hands-on Python script

No, while this would be the ideal solution, the current app does not handle errors raised from the download script (known feature gap that I will need to fix). For now, would it be possible to display a prompt for the student to continue, then calling sys.exit(1)? Similar to how we did it for this exercise autograding: https://github.com/git-mastery/exercises/blob/main/remote_control/verify.py#L31-L35

Let me know if this makes sense!

[oadultradeepfield]

@woojiahao I have implemented the suggested changes. Please review again to see if they make sense. I have also tested this script again, and the message appears as expected, aborting correctly.

I am okay with 2(b) as well. The caveat here is that you need to ensure that when the user runs gitmastery progress sync off, they should see this prompt to delete the progress fork. The good thing is that the app already defines deleting repositories as a central function: https://github.com/git-mastery/app/blob/main/app/utils/gh_cli.py#L83-L84. This would involve a separate PR down the line, but I am okay with tracking this as a follow-up issue.

For now, I still don’t have the clearest picture of what you’re trying to point out, but I can look into it as a separate issue in more detail if you’d like.

[woojiahao]

@oadultradeepfield Cool! I'll take a look at it in a bit. I'm currently seeing if it makes more sense for the app to handle this as a more first-class principle so that we don't need to have this hacky workaround. We are seeing this issue crop up more and more with other hands-ons as well, so would like to converge to have a common pattern for everyone

[woojiahao]

Exceptions are now raised as a first-class concept through the app. I am going to provision utility functions directly so that this behavior is not subject to any variations.

The core behavior I'm going to include as utility functions:

1. Retrieving the username of the current user
2. Deleting a repository
3. Forking a repository
4. Forking + cloning a repository
5. Cloning a repository

[oadultradeepfield]

So I guess this part can be simplified to raising an exception with a similar message here?

[oadultradeepfield]

I suppose this can be changed to get the output from the utility function you will provide as well!