Sync project specs with Grades endpoint#86
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to a9e87c7 in 54 seconds
More details
- Looked at
295lines of code in4files - Skipped
1files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. Gemfile:7
- Draft comment:
The version constraint for thezipgem is too broad. It's recommended to specify a more restrictive version to ensure compatibility and avoid potential issues with future versions of the gem.
gem "zip", "~> 2.0"
- Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
2. grade_runner.gemspec:48
- Draft comment:
The version constraint for thezipgem is too broad. It's recommended to specify a more restrictive version to ensure compatibility and avoid potential issues with future versions of the gem.
s.add_runtime_dependency(%q<zip>.freeze, ["~> 2.0".freeze])
- Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
3. lib/tasks/grade.rake:35
- Draft comment:
The submission URL is set to a ngrok URL, which is likely for testing purposes. Ensure to revert this to the production URL before merging.
submission_url = "https://grades.firstdraft.com"
- Reason this comment was not posted:
Marked as duplicate.
4. lib/tasks/grade.rake:111
- Draft comment:
The submission URL is set to a ngrok URL, which is likely for testing purposes. Ensure to revert this to the production URL before merging.
submission_url = "https://grades.firstdraft.com"
- Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_4f22WGo5UF3JFGzL
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5b665ab in 36 seconds
More details
- Looked at
37lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. lib/tasks/grade.rake:233
- Draft comment:
The PR description mentions updates tosync_specs_with_sourcefor handling zip files, but the diff does not reflect these changes. Please ensure that the intended updates are correctly implemented. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_RbUF4zAAGiBPu2RX
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4a3823c in 48 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. lib/tasks/grade.rake:143
- Draft comment:
The PR description mentions updates tosync_specs_with_sourcefor handling zip files, but the diff does not reflect these changes. Please ensure that all intended changes are included in the PR. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_XEv1hxkB8tqx9WZ6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 20f9339 in 1 minute and 5 seconds
More details
- Looked at
31lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. lib/tasks/grade.rake:23
- Draft comment:
The submission URL here does not match the one mentioned in the PR description (https://2fe2-149-75-206-231.ngrok-free.app). Please update it to ensure consistency.
student_config["submission_url"] = "https://2fe2-149-75-206-231.ngrok-free.app"
- Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
2. lib/tasks/grade.rake:35
- Draft comment:
The submission URL here also does not match the one mentioned in the PR description (https://2fe2-149-75-206-231.ngrok-free.app). Please update it to ensure consistency.
submission_url = "https://2fe2-149-75-206-231.ngrok-free.app"
- Reason this comment was not posted:
Marked as duplicate.
3. lib/tasks/grade.rake:112
- Draft comment:
The submission URL here also does not match the one mentioned in the PR description (https://2fe2-149-75-206-231.ngrok-free.app). Please update it to ensure consistency.
submission_url = "https://2fe2-149-75-206-231.ngrok-free.app"
- Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_6HIwl5M4W4ahCqWA
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
bpurinton
force-pushed
the
jw-download-specs-from-grades
branch
from
20f9339 to
4a3823c
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 07eb633 in 1 minute and 19 seconds
More details
- Looked at
142lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. lib/tasks/grade_runner.rake:1
- Draft comment:
The PR description mentions updates to howgrade_runnerretrieves spec folder SHA and project source code URL, but these changes are not reflected in the provided code. Please ensure all intended changes are included in the PR. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
2. lib/tasks/grade.rake:137
- Draft comment:
The PR description mentions updates to howgrade_runnerhandles ZIP files for spec synchronization, but these changes are not reflected in the provided code. Please ensure all intended changes are included in the PR. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_w0i1vWqNaWZmUe21
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| end | ||
| end | ||
|
|
||
| def sync_specs_with_source(full_reponame, remote_sha, repo_url) |
There was a problem hiding this comment.
Perhaps related to some of the changes on this method. As a student, I deleted a one spec, then staged (but did not commit) the file. I then ran grade, and I seemed to have tricked Grades into thinking there was one less spec than before. I'm not sure if this was actually behavior that occurred prior to this PR as it only just occurred to me to test this somewhat edge-case (we never describe / use "staging" for students and just have them select "Always" the first time they commit without staging)..
Student codespace:
Tricked grades:
There was a problem hiding this comment.
And just want to confirm that if I actually commit the changes (or just don't stage in the first place), it does the right thing and downloads the "official" spec folder again.
There was a problem hiding this comment.
Oh good catch. I hadn't tested the case where the SHA of the local spec folder matches the remote and a spec file change has been staged. This was a problem previously but can be handled if grade runner unstages staged files from the spec folder before comparing SHA's.
| FileUtils.rm(project_root.join("tmp/spec.zip")) | ||
| FileUtils.rm_rf(extracted_zip_folder) | ||
| `git add spec/` | ||
| `git commit spec/ -m "Update project tests" --author "First Draft <grades@firstdraft.com>"` |
There was a problem hiding this comment.
| `git commit spec/ -m "Update project tests" --author "First Draft <grades@firstdraft.com>"` | |
| `git commit spec/ -m "Update spec folder to match official version" --author "First Draft <grades@firstdraft.com>"` |
Or maybe something like that? Just to make it explicit via the commit message to students that this was caused by them attempting to edit the spec/ folder or us making an update to the specs? I hesitate to use a word like "upstream" even though that is correct, as it might not be clear to beginners.
There was a problem hiding this comment.
Related, this message on the commit is a little curious. Are we sure we want to use that grades@firstdraft.com email? To be clear, I don't think it matters having this little message, but it just caught my eye and wanted to point it out.
There was a problem hiding this comment.
huh, interesting. I didn't think that was an email GitHub would recognize since there isn't a GitHub account with that email... 🤔
My intention was to have "First Draft" make the commit so it's clear to the student who made the commit. As long as it doesn't break anything, I think it's fine for now.
There was a problem hiding this comment.
I think it looks fine with that email and on second though it kind of makes sense (the commit isn't from them, and GitHub is letting them know; and the grades email makes it clear it's coming from them running the rake grade command and receiving some updates).
My other point about the more informative commit message still stands though.
bpurinton
force-pushed
the
jw-download-specs-from-grades
branch
from
07eb633 to
4a3823c
Compare
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 73f5033 in 1 minute and 19 seconds
More details
- Looked at
79lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. lib/tasks/grade.rake:139
- Draft comment:
The PR description states that thegrade_runnerretrieves spec folder SHA and URL from the Grades API endpoint, but the code does not show this interaction. Please ensure that the API interaction is correctly implemented or update the PR description if the current implementation is as intended. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
2. lib/tasks/grade.rake:161
- Draft comment:
The commit author is hardcoded as 'First Draft grades@firstdraft.com'. Consider using the actual user's commit details or making the author configurable to better suit different environments. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_BdMk7px70HNpXJpS
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e4a63be in 1 minute and 18 seconds
More details
- Looked at
42lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. lib/tasks/grade.rake:145
- Draft comment:
The functionsync_specs_with_sourceis expected to interact with the Grades API to fetchremote_shaandrepo_url, but it currently does not show this interaction. Please ensure that the API call to fetch these details is implemented correctly or clarify if it's handled elsewhere. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_DlqJrD6B3GvocC7p
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| config = YAML.load_file(config_file_name) | ||
| if config["github_username"].present? | ||
| return config["github_username"] | ||
| end |
There was a problem hiding this comment.
Is it ever the case that the username is written to the .ltici_apitoken.yml file? I don't see where in the logic this would be happening, and in my testing I don't see it being written there. Just wondering if / why this is necessary.
There was a problem hiding this comment.
Good catch. I think I accidentally removed this from regularly running rake grade. It does get set when running rake grade:reset_token but I'll make sure it gets set on the first grade task.
bpurinton
left a comment
bpurinton
left a comment
There was a problem hiding this comment.
Left one comment about the user of username. A small explanation there would suffice. In my testing everything worked perfectly, so feel free to merge anytime! 🚢
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on bfe3b57 in 51 seconds
More details
- Looked at
28lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. lib/tasks/grade.rake:21
- Draft comment:
The PR description mentions significant changes to how thegrade_runnerinteracts with the Grades API and handles spec synchronization, but the provided diff does not reflect these changes. Please ensure the correct files and changes are included in the PR. - Reason this comment was not posted:
Confidence of 50% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_t31rzMKNBdoCRiuP
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
After merging and deploying, I pushed the new grade_runner to ruby gems and used a grades token from an active lesson and everything worked as expected. Projects can be updated with the new grade_runner version. |
Done! |
2 participants
Resolves #75
Related to https://github.com/firstdraft/grades/pull/736
Grades PR includes testing instructions
This PR makes the following changes:
Summary:
Updated
grade_runnergem to sync project specs with the Grades endpoint using zip files and added necessary functions for handling zip file operations.Key points:
zipgem toGemfileandgrade_runner.gemspec0.0.12inVERSIONandgrade_runner.gemspeclib/tasks/grade.raketo sync specs with the Grades endpointdownload_file,extract_zip, andoverwrite_spec_folderto handle zip file operationssync_specs_with_sourceto use the new zip file handling functionshttps://2fe2-149-75-206-231.ngrok-free.appfind_or_create_directoryfunctionGenerated with ❤️ by ellipsis.dev