Skip to content

First one for demo #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SyedArshikJavedMoinee wants to merge 8 commits into
base: main
Choose a base branch
from
Open

First one for demo #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c76e94e
First one for demo
SyedArshikJavedMoinee Jun 13, 2024
9af595a
First one for demo
SyedArshikJavedMoinee Jun 13, 2024
4b83e9b
First one for demo
SyedArshikJavedMoinee Jun 13, 2024
3d9b4ca
First one for demo
SyedArshikJavedMoinee Jun 13, 2024
6b97fc1
First one for demo
SyedArshikJavedMoinee Jun 13, 2024
6fb2dc7
First one for demo
SyedArshikJavedMoinee Jun 13, 2024
662bf93
First one for demo
SyedArshikJavedMoinee Jun 13, 2024
052577e
First one for demo
SyedArshikJavedMoinee Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 7 additions & 27 deletions .github/workflows/pr-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,32 +13,10 @@ jobs:
steps:
- name: Checkout repository
uses: actions/checkout@v3

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v2

- name: Cache Docker layers
uses: actions/cache@v3
with:
path: /tmp/.buildx-cache
key: ${{ runner.os }}-docker-${{ github.sha }}
restore-keys: |
${{ runner.os }}-docker-


- name: Pull Docker image
run: |
docker buildx create --use
docker pull arshikjaved/pr-review:v1.0 || true
docker buildx build --cache-from=type=local,src=/tmp/.buildx-cache \
--cache-to=type=local,dest=/tmp/.buildx-cache-new \
-t arshikjaved/pr-review:v1.0 .

- name: Update Docker cache
if: success()
run: |
rm -rf /tmp/.buildx-cache
mv /tmp/.buildx-cache-new /tmp/.buildx-cache

run: docker pull arshikjaved/pr-review:v1.0

- name: Set script name
id: script_name
run: |
Expand All @@ -59,7 +37,7 @@ jobs:
LINE_NUMBER: ${{ github.event.comment.line }}
REPOSITORY_OWNER: ${{ github.repository_owner }}
REPOSITORY_NAME: ${{ github.event.repository.name }}
SCRIPT: ${{ steps.script_name.outputs.script }}
SCRIPT: ${{ github.event_name == 'pull_request' && 'generate_response.py' || 'reply_thread.py' }}
COMMIT_SHA: ${{ github.event.pull_request.head.sha }}
PR_NUMBER: ${{ github.event.pull_request.number }}

Expand All @@ -68,4 +46,6 @@ jobs:
docker run --rm -e OWNER='${{env.REPOSITORY_OWNER}}' -e REPO_NAME='${{env.REPOSITORY_NAME}}' -e COMMIT_SHA="${{ github.event.pull_request.head.sha }}" -e PR_NUMBER="${{ github.event.pull_request.number }}" -e ACTION="${{env.ACTION}}" -e EVENT_NAME="${{ env.EVENT_NAME }}" arshikjaved/pr-review:v1.0 sh -c "python /app/generate_response.py --owner '${{env.REPOSITORY_OWNER}}' --repo-name '${{env.REPOSITORY_NAME}}' --commit-sha '${{ env.COMMIT_SHA }}' --pr-number '${{ env.PR_NUMBER }}' --event-name '${{ env.EVENT_NAME }}' --action '${{ env.ACTION }}'"
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docker run command is not checking if the environment variables are set before using them. This can lead to errors if the variables are not set. Consider adding checks to ensure the variables are set before using them.

else
docker run --rm -e OWNER='${{env.REPOSITORY_OWNER}}' -e REPO_NAME='${{env.REPOSITORY_NAME}}' -e COMMIT_SHA="${{ github.event.pull_request.head.sha }}" -e PR_NUMBER="${{ github.event.pull_request.number }}" -e EVENT_NAME="${{ env.EVENT_NAME }}" arshikjaved/pr-review:v1.0 sh -c "python /app/reply_thread.py --owner '${{env.REPOSITORY_OWNER}}' --repo-name '${{env.REPOSITORY_NAME}}' --commit-sha '${{ env.COMMIT_SHA }}' --pr-number '${{ env.PR_NUMBER }}' --event-name '${{ env.EVENT_NAME }}' --comment-body '${{ env.COMMENT_BODY }}' --comment-id '${{ env.COMMENT_ID }}' --file-path '${{ env.FILE_PATH }}' --line-number '${{ env.LINE_NUMBER }}'"
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docker run command is not checking if the script exists before running it. It's possible that the script might not exist in the container, which would cause the command to fail. Consider adding a check to ensure the script exists before running it.

fi
fi


4 changes: 2 additions & 2 deletions mern-ecommerce/controllers/category.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ exports.update = (req, res) => {
category.save((err, data) => {
if (err) {
return res.status(400).json({
error: errorHandler(err),
error: errorHandlerr(err),
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in errorHandlerr, it should be errorHandler.

});
}
res.json(data);
Expand All @@ -59,7 +59,7 @@ exports.remove = (req, res) => {
};

exports.list = (req, res) => {
Category.find().exec((err, data) => {
Categoryy.find().exec((err, data) => {
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in Categoryy, it should be Category.

if (err) {
return res.status(400).json({
error: errorHandler(err),
Expand Down
6 changes: 3 additions & 3 deletions mern-ecommerce/routes/auth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ const {
} = require('../controllers/auth');
const { userSignupValidator } = require('../validator');

router.post('/signup', userSignupValidator, signup);
router.post('/signin', signin);
router.get('/signout', signout);
router.post('/signup', userSignupValidator, signupss);
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

signupss is not defined in this file, but signup is imported from ../controllers/auth. It seems like there's a typo and it should be signup instead of signupss.

Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function signupss is not defined in the provided context. It seems like it should be signup instead of signupss.

router.post('/signin', signinss);
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

signinss is not defined in this file, and it's not imported from anywhere. It should be signin instead of signinss.

Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function signinss is not defined in the provided context. It seems like it should be signin instead of signinss. The correct line should be router.post('/signin', signin);.

Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CodeHawk Can you please this in simpler terms. Like very simple terms. Pretend im not a developer

Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a simple explanation: You've written signinss instead of signin. It should be router.post('/signin', signin); because signinss is not defined anywhere.

router.get('/signout', signoutss);
Copy link
Copy Markdown
Collaborator Author

@SyedArshikJavedMoinee SyedArshikJavedMoinee Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

signoutss is not defined in this file, but it's being used as a callback function. It should be signout instead, which is imported from ../controllers/auth.


module.exports = router;
1 change: 0 additions & 1 deletion mern-ecommerce/routes/category.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ router.delete(
remove
);
router.get('/categories', list);

router.param('categoryId', categoryById);
router.param('userId', userById);

Expand Down