Skip to content
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

fix: works when Gitlab runner is not at the same machine as Gitlab #289

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

Conversation

softaria-roman
Copy link

Hello,

Thank you for this great product.
This little patch fixes the following issue with Gitlab integration:
Currently, it uses localhost to communicate with GitLab via API.
While in most real-life installations, gitlab runners and gitlab itself run on different machines,
So, localhost does not work.
Fortunately, gitlab has a special environment variable with its url.
The patch uses it. The patch was tested in a real environment.

Copy link
Owner

Choose a reason for hiding this comment

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

Please remove this lock file

@@ -11,7 +11,7 @@
"lint-fix": "eslint . --ext=js,ts,tsx --fix",
"lint-test": "eslint . --ext=js,ts",
"start": "ts-node ./src/index.ts",
"review": "ts-node ./src/index.ts review",
"review": "ts-node ./src/index.ts review --model=gpt-3.5-turbo --debug=true",
Copy link
Owner

Choose a reason for hiding this comment

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

please remove this

DiscussionNotePositionOptions,
Gitlab,
} from "@gitbeaker/rest";
// eslint-disable-next-line import/no-extraneous-dependencies
Copy link
Owner

Choose a reason for hiding this comment

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

why do you need this?

@mattzcarey
Copy link
Owner

Hey thanks very much for your contribution. I love the better GitLab implementation. Unfortunately, I can't merge it as is.

  • Please can you clean up the readmes?
  • If you no longer want to show the emoji summary can you add it as a flag to switch it off?
  • Similar to showing all reviews. This should be a flag. Imagine changing 30 files. It will become unreadable.
  • Removing the decoding of the answer means that whenever there is a code block suggested, that feedback will fail. This is not the desired functionality.

Many thanks, Matt

@softaria-roman
Copy link
Author

Sorry.
I intended to send only the first commit (which fixes one bug) not all 3 (with a lot of changes).
Please close this PR and I will send new with the only required things.

I guess I will send 2 PRs - one with the bug fixes and another one with the gitlab staff.

I will not touch emoji and other things.

Will it be okay?

@mattzcarey
Copy link
Owner

I still cant merge this. Please can you check the diff.

@borjafv
Copy link

borjafv commented Jan 12, 2024

Any news about this? It would be great to have a better Gitlab integration. Thanks!

@softaria-roman
Copy link
Author

Any news about this? It would be great to have a better Gitlab integration. Thanks!

Sorry, I am really too busy now :(
I keep it in mind.

Also, feel free to just get any code you find useful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants