-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add GitLab functionalities #1098
Conversation
add code to handle Gitlab repository, add axios to project packages, add env variables for Gitlab
Here the list of file changed or added in order to add the ability to use GitLab instead of GitHub as store repo. For each file, a simple description is given to describe its purpose within the application and the changes made for GitLab: package.json (and package-lock.json) |
Hello, @fabianospinelli @alessandrozago could you please clarify what is the relationship between #1095 and #1098, and close the potentially obsolete PR? The European Commission that employs you certainly provides enough resources to minimise the burden on maintainers 🙂 In general, introducing yourselves and your intentions is a welcome first step in open source collaboration before requesting maintainers to allocate time to review your work. You can find a good introduction to open source collaboration on the Open Source Guide, in particular for the case at hand in section 5, “How to submit a contribution”:
I am happy to have a video call with your team if useful. |
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.
Since merging code into the codebase entails long-term maintenance commitment, we will also need automated tests to pass before considering review.
If you'd like to speed up the review process for this GitLab integration to be available within your organisation, please consider sponsoring!
config/default.json
Outdated
@@ -59,6 +59,7 @@ | |||
"dataset": { | |||
"title": "sandbox", | |||
"versionsRepositoryURL": "https://github.com/OpenTermsArchive/sandbox", | |||
"versionsRepositoryURLGitLab": "https://gitlab.com/p2b/contrib-versions", |
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.
The account p2b
on GitLab is reserved. Do you own it? If not, it does not seem appropriate to send notifications to that account by default.
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.
The url was only used as an example, but it was our mistake since we did not realize it was pointing to an existing account. This was removed, and replaced with an obvious fake url as an example. It will be available when the pull request will be updated.
package.json
Outdated
@@ -100,7 +100,8 @@ | |||
"swagger-jsdoc": "^6.2.8", | |||
"swagger-ui-express": "^5.0.0", | |||
"winston": "^3.3.3", | |||
"winston-mail": "^2.0.0" | |||
"winston-mail": "^2.0.0", | |||
"axios": "^1.7.2" |
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.
Adding a dependency is not a lightweight change as it introduces security risks and maintenance burden. It seems this dependency is used only for 3 HTTP calls. Please use the native http
module or one of the existing dependencies.
Please also use npm install --save
to maintain alphabetical order and avoid future churn.
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.
This is solved and will be available when the pull request will be updated. Axios was removed and was replaced with node-fetch (already used in the project).
|
||
import config from 'config'; | ||
import dotenv from 'dotenv'; | ||
//import { Octokit } from 'octokit'; |
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.
Do not keep commented-out lines, please erase them.
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.
This is solved (along with other similar cases) and will be available when the pull request will be updated
Hi @MattiSG , thank you for the feedback and the resources provided. |
src/reporterGitlab/labels.test.js
Outdated
expect(descriptionLength).to.be.lessThan(GITLAB_LABEL_DESCRIPTION_MAX_LENGTH); | ||
}); | ||
|
||
it('complies with the GitHub constraints for color', () => { |
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.
This still mentions GitHub when it's related to GitLab 😉
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.
This is solved and will be available when the pull request will be updated.
src/reporterGitlab/labels.json
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.
Do you really need to duplicate this file? It seems to be a complete duplicate of the GitHub reporter labels. Why not simply import the same file? 🙂
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.
The color format is different; although it is just the "#" character at the beginning of the color code.
We could find some way to use the same test file and simply add the "#" character via code for GitLab, but I do not think this is a good solution.
Considering GitHub and GitLab use different formats, it's also possible they might change in the future. To us, it makes sense to have them separate.
Please let us know if you prefer to unify the tests anyway. In this case, it might also make sense to move the "labels.json" in a more generic directory/path.
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.
This seems to be a complete duplicate from the GitHub template. Duplicating this file means that they are more likely to not be updated properly. Is there a reason for not simply importing the existing template? 🙂
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.
The only change in that file is, in fact, just a variable name (compared to the GitHub counterpart).
To use a single template, we could:
- pass that variable as an input for the template and modify the code in the Release generation to handle both the GitHub and GitLab parts
- use only one variable in the configuration file for "versionsRepositoryURL"; this will make the publishing part not consistent with the reporting part, where GitHub and GitLab use different configurations (reporter.githubIssues and reporter.gitlabIssues).
Please let us know if there is a preferred solution, otherwise we could proceed with n°1.
add test file for GitLab, fix formatting in files, refactor GitLab code to remove axios library
remove the GitLab releases repository with a new one used as an example
remove duplicate readme for gitlab and handle the new input parameter
Fix the release zip workflow using GitLab packages to upload the file
aligned format and minor wording fixes
Integrated by #1116 |
add code to handle Gitlab repository, add axios to project packages, add env variables for Gitlab