-
Notifications
You must be signed in to change notification settings - Fork 12
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.
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
Issue #320: Check total amount on done in Add Details pop-up #408
Conversation
EPL-1160
517cb27
to
0855a41
Compare
..._core/org.openbravo.advpaymentmngt/web/org.openbravo.advpaymentmngt/js/ob-aprm-addPayment.js
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
..._core/org.openbravo.advpaymentmngt/web/org.openbravo.advpaymentmngt/js/ob-aprm-addPayment.js
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
.github/workflows/auto-reviewer.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GPT Review for auto-reviewer.yml
Review
- Estimated effort to review [1-5]: 2, because the changes are limited to a configuration file and involve conditional logic adjustments, which are straightforward but require careful validation.
- Score: 95
Code feedback
- File: .github/workflows/auto-reviewer.yml
- Language: yaml
- Suggestion: Ensure that the
workflow_run
event triggers correctly by verifying the workflow name and event type. This is important to avoid unintended behavior. [important] - Label: best practice
- Existing code:
on:
workflow_run:
workflows: [Git Police]
types:
- completed
- Improved code:
on:
workflow_run:
workflows: ["Git Police"]
types:
- completed
.github/workflows/git-police.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GPT Review for git-police.yml
Review
- Estimated effort to review [1-5]:
2, because the PR adds a new GitHub Actions workflow file. The logic is straightforward but involves multiple steps and environment variables, which need careful review. - Score: 85
Code feedback
- File:
.github/workflows/git-police.yml - Language:
yaml - Suggestion:
Consider adding a step to clean up the Docker container after the API call. This ensures that no unnecessary containers are left running, which could consume resources and potentially cause conflicts in future runs. [important] - Label:
best practice - Existing code:
- name: Git Police API call
id: call-api
run: |
sleep 10s
response=$(curl -s -o response.txt -w "%{http_code}" -X GET http://localhost:5000/api/github)
if [ $response -ne 200 ]; then
echo "API call failed with status code $response. This is likely an error related to the PR data, or an internal API error"
echo "Response text:"
cat response.txt
exit 1
fi
if ! grep -q "Valid branch name and correct destination" response.txt; then
echo "Git Police check failed: response does not contain 'Valid branch name and correct destination'"
echo "Full response:"
cat response.txt
exit 1
fi
echo "API call successful with status code $response"
echo "Response text:"
cat response.txt
- Improved code:
- name: Git Police API call
id: call-api
run: |
sleep 10s
response=$(curl -s -o response.txt -w "%{http_code}" -X GET http://localhost:5000/api/github)
if [ $response -ne 200 ]; then
echo "API call failed with status code $response. This is likely an error related to the PR data, or an internal API error"
echo "Response text:"
cat response.txt
exit 1
fi
if ! grep -q "Valid branch name and correct destination" response.txt; then
echo "Git Police check failed: response does not contain 'Valid branch name and correct destination'"
echo "Full response:"
cat response.txt
exit 1
fi
echo "API call successful with status code $response"
echo "Response text:"
cat response.txt
- name: Clean up Docker container
run: |
docker ps -q --filter "ancestor=etendo/gitpolice:latest" | xargs -r docker stop
docker ps -a -q --filter "ancestor=etendo/gitpolice:latest" | xargs -r docker rm
..._core/org.openbravo.advpaymentmngt/web/org.openbravo.advpaymentmngt/js/ob-aprm-addPayment.js
Show resolved
Hide resolved
.github/workflows/auto-reviewer.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GPT Review for auto-reviewer.yml
Review
- Estimated effort to review [1-5]: 2, because the changes are straightforward but involve understanding GitHub Actions and the workflow logic.
- Score: 90
Code feedback
- File: .github/workflows/auto-reviewer.yml
- Language: yaml
- Suggestion: Ensure that the workflow is triggered only when the
Git Police
workflow completes successfully. This change enhances the conditional logic to prevent unnecessary runs. [important] - Label: enhancement
- Existing code:
workflow_run:
workflows: [Git Police]
types:
- completed
- Improved code:
workflow_run:
workflows: ["Git Police"]
types:
- completed
.github/workflows/git-police.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GPT Review for git-police.yml
Review
- Estimated effort to review [1-5]: 2, because the PR introduces a new GitHub Actions workflow, which is relatively straightforward but requires careful review to ensure correct environment variables and API calls.
- Score: 85
Code feedback
-
File: .github/workflows/git-police.yml
-
Language: yaml
-
Suggestion: It's a good practice to mask sensitive information like tokens and passwords in logs to avoid accidental exposure. You can use the
::add-mask::
command to achieve this. [important] -
Label: security
-
Existing code:
printenv echo "-------------------------------------------------------------------------" echo ${{ env.DOCKERHUB_PASSWORD }} | docker login --username "${{ env.DOCKERHUB_USERNAME }}" --password-stdin
-
Improved code:
printenv echo "-------------------------------------------------------------------------" echo "::add-mask::${{ env.DOCKERHUB_PASSWORD }}" echo ${{ env.DOCKERHUB_PASSWORD }} | docker login --username "${{ env.DOCKERHUB_USERNAME }}" --password-stdin
-
File: .github/workflows/git-police.yml
-
Language: yaml
-
Suggestion: Adding a timeout to the
sleep
command can help avoid indefinite waiting in case of issues with the API. [medium] -
Label: best practice
-
Existing code:
sleep 10s
-
Improved code:
timeout 30s sleep 10s
..._core/org.openbravo.advpaymentmngt/web/org.openbravo.advpaymentmngt/js/ob-aprm-addPayment.js
Show resolved
Hide resolved
Warning Git Police 👮One or more commit messages do not meet the required standards. Please correct them.
|
Warning Git Police 👮One or more commit messages do not meet the required standards. Please correct them.
|
EPL-1160
Warning Git Police 👮One or more commit messages do not meet the required standards. Please correct them.
|
8da1492
to
64e7fe1
Compare
This comment has been minimized.
This comment has been minimized.
Warning Git Police 👮One or more commit messages do not meet the required standards. Please correct them.
|
EPL-1160