Closed nknguyenhc closed 7 months ago
Attention: 32 lines
in your changes are missing coverage. Please review.
Comparison is base (
d4b5422
) 53.62% compared to head (0cd1148
) 53.41%.
Files | Patch % | Lines |
---|---|---|
src/app/core/services/github.service.ts | 13.04% | 20 Missing :warning: |
src/app/core/services/upload.service.ts | 0.00% | 9 Missing and 3 partials :warning: |
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Thanks @cheehongw for spotting the bug! Can I check the state of the repo when the error occurred, in particular,
I can only reproduce the error when the repo already has some images, and the default branch of the repo is not main
. When the repo is empty, even if your default branch in your Github settings is not main
, the file uploaded will be in main
branch.
I have considered the following solutions:
main
branch upon repo creation.Note that we cannot create a new branch in an empty repository, because branches are only pointers to commits, we would have to create a commit in order to create the branch.
Octokit allows us to create commit, but from the documentation, it does not look like we can indicate the branch upon creating the commit.
Octokit has the option auto_init
upon repo creation, however, it looks like we cannot indicate the branch for the initial commit.
Another approach is to upload a dummy file using the same update file API (indicating the branch name to create the branch).
I believe this solution is not ideal as it adds more API calls, which can cause Github to block our access during the actual PE. Furthermore, this does not address the concern above, as we cannot create a branch out of an empty repository, for the first image upload.
According to the PE instructions, students should only be doing their PE through CATcher interface, so I believe it should be save to assume that repo starts out as an empty repo, and hence, it should be possible to indicate the branch name for updating files? Otherwise, if we were to be safe, should I proceed with creating a dummy upon repo creation?
Thanks for the investigation.
- Is the repo empty or does it already have some images?
- If it already has some images, what is the default branch of the repo?
When I tested it, the default branch was master
with some commits. The main
branch does not exist.
So for the initial commit, is it possible to specify the initial branch name to be whatever we want?
I believe this solution is not ideal as it adds more API calls.
I agree. Perhaps we can use an optimistic processing approach instead? In other words, upload the image and then take corrective action if the upload fails due to missing branch
I think it is fine to assume that users start out with an empty repository. https://nus-cs2103-ay2324s2.github.io/website/schedule/week10/project.html#5-smoke-test-catcher-compulsory-counted-for-participation
So for the initial commit, is it possible to specify the initial branch name to be whatever we want?
Yes, we can use upload file API call to ensure that the default branch is main
. However, it only works if the repo is empty; if the repo already has some commits, we have to branch out from the latest commit. This means when a user logs in, we have to handle 3 cases separately:
main
for empty repo.main
.main
.I believe that for PE, if students follow instructions and do the PE using CATcher, the only way that it falls under 2nd or 3rd case is when they have previously created a repo with the exact same name (i.e. alpha
, ped
or pe
).
Perhaps we can use an optimistic processing approach instead? In other words, upload the image and then take corrective action if the upload fails due to missing branch
I would prefer this solution. I am thinking about if the upload fails, branch out from the latest commit in the repo, naming the new branch main
, and retry the upload.
Would love to hear what you think about my suggested solution!
- Create an initial commit by uploading a dummy file to branch
main
for empty repo.
I tried creating a commit to a empty repo using the createOrUpdateFile
API, and specifying the branch name as x
. In the empty repo, the branch x
is made, so it seems like there is no need to upload a dummy file for an empty repo with the branch we want. I think your PR as it is now should achieve this?
- Branch out from latest commit for non-empty repo that does not have branch
main
.
We should do this when we optimistically upload the image file to main
.
-Do nothing for non-empty repo that has branch
main
.
Right, because the branch we are uploading to exists, so there's nothing to do.
I have updated my PR! So in summary, the changes are only when user uploads a file:
main
.main
from the latest commit and retry.
Summary:
Fixes CATcher-org/CATcher#1226
Changes Made:
When uploading image, indicate the branch to upload to be
main
. Since there are no other github API used that makes commits, this means that default branch will be set tomain
for the first image upload.We do optimistic processing of uploads, in the case that the repository is not empty. When the upload fails, we create
main
branch from the latest commit in the repository, and re-attempt the upload.Proposed Commit Message: