Add support for Gitlab #35
Conversation
There was a problem hiding this comment.
This is great. Thanks for opening up this change. I would recommend breaking this up into two changes. There are the quality of life changes like the docker build stages update, the cleaner logging utilities, and prompt character fixes, which I can approve immediately.
For the the other stuff like the new docker workflow and gitlab tweaks I just had a couple questions
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Log in to the Container registry | ||
| uses: docker/login-action@65b78e6e13532edd9afa3aa52ac7964289d1a9c1 |
There was a problem hiding this comment.
Why the hash after the @? I would expect some version number on these actions
There was a problem hiding this comment.
No real reason, just used what was recommended from GitHub documentation page:
https://docs.github.com/en/actions/publishing-packages/publishing-docker-images#publishing-images-to-github-packages
# GitHub recommends pinning actions to a commit SHA.
# To get a newer version, you will need to update the SHA.
I'll make change to update to versions instead of commit SHA.
| @@ -0,0 +1,36 @@ | |||
| name: Create and publish a Docker image | |||
There was a problem hiding this comment.
The currently workflow I follow is to publish a release and Github will host that version for us. What's the intent behind having the docker image workflow? Is this more relevant for gitlab?
There was a problem hiding this comment.
That is correct. GitLab needs to have an image we can reference for the CI/CD job to start. You can see the reference in README.md part, here:
image:
name: ghcr.io/Integral-Healthcare/robin-ai-reviewer
entrypoint: [""]If we don't have access to this specific image, we would have to resort to using an image that already has Git installed. Then, we'd need to clone this repository and perform the build within the runner. However, this approach would necessitate the user having Docker-in-Docker (DinD) support in the runner, as well as Git and Docker installed. The recommended workflow, on the other hand, is to mount an image that contains the tools the user wants to use. Then, within the CI process, we can execute scripts from within that mounted image. In this specific tool's case, we would initiate the 'entrypoint.sh' script from the runner.
I also believe this approach can save time during the build phase. If GitHub workflows were to adopt this method, it would eliminate the need for DinD in the GitHub runner when building the image. Furthermore, using a lighter image should result in faster job start times. I haven't had direct experience with GitHub Actions, so I can't confirm if this practice is widely adopted or if people typically build images from within the runners for their workflows.
| @@ -1,10 +1,18 @@ | |||
| FROM alpine:3.15 | |||
| FROM alpine:3.18 as docpars | |||
There was a problem hiding this comment.
sweet, thanks for doing this multi-stage docker update
| | `files_to_ignore` | No | (empty string) | A whitespace delimited list of files to ignore. | | ||
| | Name | Required | Default Value | Description | | ||
| |--|--|--|--| | ||
| | `open_ai_api_key` | Yes | N/A | An API key from Open AI's [developer portal](https://platform.openai.com/account/api-keys). | |
There was a problem hiding this comment.
Intention for the all caps OPEN_AI_API_KEY was to emphasize that it was a required argument
There was a problem hiding this comment.
Understood, I can change it here to have capital letters, but what about the usage from within the script? Should it also accept OPEN_AI_API_KEY as a parameter, or for consistency, stay as open_ai_api_key? It might be confusing if documentation is showing capitalized parameter while the entrypoint script accepts lower case argument.
There was a problem hiding this comment.
Whatever we elect for, we should be consistent. We should update the entry point script to accept an upper case argument in my opinion
| echoerr "API request failed: $error" | ||
| exit 1 | ||
| if [ "$error" != "null" ]; then | ||
| kill -s TERM $TOP_PID |
There was a problem hiding this comment.
Does kill -s TERM $TOP_PID present an issue if $TOP_PID env var doesn't exist?
There was a problem hiding this comment.
Oh I see you're explicitly setting it here https://github.com/Integral-Healthcare/robin-ai-reviewer/pull/35/files#diff-e4d34fc353097073f2f6ebbdc1d89cbdac9f0edf5354442c2f884839a2e8afb3R4
There was a problem hiding this comment.
Had to add this so we can exit the script when something fails in the downstream. Previously, error from within the function would still continue to execute the main script and report it as a success. So, for example, if the API key for OpenAI was incorrect, it would print the error message and still report workflow/job as successful since the main script executed successfully. Maybe that is not an issue in Github actions, but in GitLab it would falsely report the result in the UI.
There was a problem hiding this comment.
Interesting, that sounds like a gitlab issue. So long as it doesn't introduce issues with the github action flow, it seems reasonable enough
There was a problem hiding this comment.
I think GitHub workflow works the same, for example, here you have an error from bash script but it continued to show successful execution:
https://github.com/Integral-Healthcare/robin-ai-reviewer/actions/runs/6723617742/job/18274076022#step:3:131
There was a problem hiding this comment.
You're completely right. Good catch!
Interesting, is it possible to define a
That could be a newly discovered bug, it seems like your adjustments to the main.sh file with |
I have added support for GitLab merge requests as well as made some minor adjustments to the code.
@johnlk Let me know if you think Gitlab support could be added to this project or should I maintain a separate fork for it (maybe the project was intended only as GitHub workflow, in that case, I'll close this PR)
Changes:
docparsinstallation to a separate layer we can save some space onwgetpackage. The final image is 15.6Mb locally and 7.8Mb when pushed to registry.git_providervariable and renamedGITHUB_TOKENtogit_tokenutils.shso they can be re-used by Gitlab and not pushed by workflowI'm not sure about the following:
files_to_ignoreto work with multiple files, I think the current workflow is only passing first file for exclusion and anything after that is ignored. I have tested it by creating a simple variable:And then running entrypoint.sh script we get
Invalid arguments.error when trying to parse variable usingdocpars.Could you verify if it is working as intended in main branch on not a bug on my side?
I'm also attaching a few screenshots of the integration from the GitLab UI.
