-
Notifications
You must be signed in to change notification settings - Fork 11
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
create gitlab connector (#37) #197
Conversation
I'm not sure what do you mean by this @BeckerFrank Since upcoming 4.0.0 is major release, we are free to add & remove whatever we want, and GitLab support could be a nice addition to 4.0.0. |
Now I have the a version with the following features.
@wimjongman, @ruspl-afed, @merks, @gnl42 : please can you give some feedback. |
Hey Frank, I am out of office for a while.
|
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.
What needs to be done:
- license headers (add)
- gitlab images (replace with some standard images and ask IP team how to proceed)
- API (avoid declaring API on this stage)
All others may be done later
...nectors/gitlab/org.eclipse.mylyn.gitlab.core/src/org/eclipse/mylyn/gitlab/core/Duration.java
Show resolved
Hide resolved
mylyn.tasks/connectors/gitlab/org.eclipse.mylyn.gitlab.core/build.properties
Outdated
Show resolved
Hide resolved
...nectors/gitlab/org.eclipse.mylyn.gitlab.core/src/org/eclipse/mylyn/gitlab/core/Duration.java
Outdated
Show resolved
Hide resolved
...lab/org.eclipse.mylyn.gitlab.core/src/org/eclipse/mylyn/gitlab/core/GitlabConfiguration.java
Show resolved
Hide resolved
...lab/org.eclipse.mylyn.gitlab.core/src/org/eclipse/mylyn/gitlab/core/GitlabConfiguration.java
Outdated
Show resolved
Hide resolved
...org.eclipse.mylyn.gitlab.core/src/org/eclipse/mylyn/gitlab/core/GitlabTaskCommentMapper.java
Outdated
Show resolved
Hide resolved
...g.eclipse.mylyn.gitlab.core/src/org/eclipse/mylyn/gitlab/core/GitlabTaskAttributeMapper.java
Outdated
Show resolved
Hide resolved
* Frank Becker - initial API and implementation | ||
*******************************************************************************/ | ||
|
||
package org.eclipse.mylyn.gitlab.core; |
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.
We need to put more work here before promoting this class to API, I suggest to move to internal
package until it will be ready.
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.
Could you use the openapi spec (https://gitlab.com/gitlab-org/gitlab/-/blob/master/doc/api/openapi/openapi.yaml?plain=1) to generate the rest client code?
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.
I will look into this in the near future how useful the generated client can be.
We can not generate the whole client because have hat to support TaskData Structure but hopefully this is only a small change.
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.
We need to put more work here before promoting this class to API, I suggest to move to
internal
package until it will be ready.
done with next commit
...g.eclipse.mylyn.gitlab.core/src/org/eclipse/mylyn/gitlab/core/GitlabRepositoryConnector.java
Outdated
Show resolved
Hide resolved
...g.eclipse.mylyn.gitlab.core/src/org/eclipse/mylyn/gitlab/core/GitlabRepositoryConnector.java
Outdated
Show resolved
Hide resolved
...lab/org.eclipse.mylyn.gitlab.core/src/org/eclipse/mylyn/gitlab/core/GitlabConfiguration.java
Show resolved
Hide resolved
...rg.eclipse.mylyn.gitlab.core/src/org/eclipse/mylyn/gitlab/core/GitlabJSonArrayOperation.java
Outdated
Show resolved
Hide resolved
...lab/org.eclipse.mylyn.gitlab.core/src/org/eclipse/mylyn/gitlab/core/GitlabNewTaskSchema.java
Outdated
Show resolved
Hide resolved
...g.eclipse.mylyn.gitlab.core/src/org/eclipse/mylyn/gitlab/core/GitlabTaskAttributeMapper.java
Outdated
Show resolved
Hide resolved
@Override | ||
public void getMultiTaskData(final TaskRepository repository, Set<String> taskIds, | ||
final TaskDataCollector collector, IProgressMonitor monitor) throws CoreException { | ||
monitor = Policy.monitorFor(monitor); |
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.
Side note, what would be needed to make IProgressMonitor autoclosable?
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 use case is not fully clear for me. Do you mean to use in try-with-resources
or something else @gnl42 ?
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 use case is not fully clear for me. Do you mean to use in
try-with-resources
or something else @gnl42 ?
Yes @ruspl-afed . So instead of:
monitor = Policy.monitorFor(monitor);
try {
monitor.beginTask("Submitting_task", IProgressMonitor.UNKNOWN);
...
} finally {
monitor.done();
}
it would be:
try (IProgressMonitor postMonitor = Policy.monitorFor(monitor)) {
postMonitor.beginTask("Submitting_task", IProgressMonitor.UNKNOWN);
...
}
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 Policy.monitorFor(monitor)
looks a bit outdated, currently we can use more modern https://github.com/eclipse-equinox/equinox/blob/master/bundles/org.eclipse.equinox.common/src/org/eclipse/core/runtime/SubMonitor.java that does not require finally
block and have a lot of other improvements.
...gitlab/org.eclipse.mylyn.gitlab.core/src/org/eclipse/mylyn/gitlab/core/GitlabTaskSchema.java
Outdated
Show resolved
Hide resolved
...b/org.eclipse.mylyn.gitlab.core/src/org/eclipse/mylyn/gitlab/core/GitlabTaskDataHandler.java
Outdated
Show resolved
Hide resolved
Sorry, I'm torn in so many different directions I can't find time for a review. 😞 |
....mylyn.gitlab.core/src/org/eclipse/mylyn/internal/gitlab/core/GitlabRepositoryConnector.java
Outdated
Show resolved
Hide resolved
mylyn.tasks/connectors/gitlab/org.eclipse.mylyn.gitlab.feature/feature.properties
Outdated
Show resolved
Hide resolved
label="Mylyn Tasks Connector: Gitlab Preview for gitlab.com only" | ||
version="4.0.0.qualifier" | ||
provider-name="Eclipse Mylyn" | ||
plugin="org.eclipse.mylyn.gitlab.core"> |
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.
here the license feature should be referenced that will bring proper license content.
mylyn.tasks/connectors/gitlab/org.eclipse.mylyn.gitlab.feature/feature.properties
Outdated
Show resolved
Hide resolved
mylyn.tasks/connectors/gitlab/org.eclipse.mylyn.gitlab.source.feature/feature.xml
Outdated
Show resolved
Hide resolved
...rs/gitlab/org.eclipse.mylyn.gitlab.ui/src/org/eclipse/mylyn/gitlab/ui/GitlabUiActivator.java
Show resolved
Hide resolved
...nectors/gitlab/org.eclipse.mylyn.gitlab.ui/src/org/eclipse/mylyn/gitlab/ui/GitlabImages.java
Outdated
Show resolved
Hide resolved
@BeckerFrank we can merge it after resolving question with GitLab images and then improve it gradually |
Gitlab images are fine, but what is with the comment for mylyn.tasks/connectors/gitlab/org.eclipse.mylyn.gitlab.source.feature/feature.xml? What is here needed?. How can I include this function in an update site without this source feature project? |
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.
Please resolve conflicts and have a look how source features represented in main
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.
LGTM
I'll look at source feature later
Here is a POC of a gitlab connector (#37) with the following features.
@wimjongman @ruspl-afed @merks WDYT?
Should we continue or wait until 4.0 is done?