No indication of ECA status on merge request
I'm trying to grab a screen shot of what it looks like when somebody who pushes a merge request has or hasn't signed the ECA.
I created a new user wbeatonn80, cloned the Handbook repository, pushed a commit, and set up a merge request.
There's no indication on the merge request page that indicates whether or not wbeatonn80 has signed the ECA.
AFAICT, the repository is correctly configured in the PMI.
Is the hook installed?
Or am I just missing something?
Activity
assigned to @malowe
@malowe can you provide guidance here?
The hook activates when a commit is made, either when you initially pushed the data up, or when you merge it into trunk. Gitlab UI shows very little of our process with how they handle hooks, so it only shows errors, otherwise, it's silent. If you push to a project repo directly, you should get the expected message validation in your command line.
@cguindon had the right of it in our other chat thread that in order to get it into the UI, we would need to use actions/workflow/pipeline stuff that needs Gitlab Runners which we don't support yet. Its why the more basic git hooks were used.
I've observed failed pushes.
$ git push origin master ... remote: GitLab: Author 'milo@bloom_picayune.com' is not a member of team To gitlab.eclipse.org:wbeaton/org.eclipse.dash.handbook.git ! [remote rejected] master -> master (pre-receive hook declined) error: failed to push some refs to 'gitlab.eclipse.org:wbeaton/org.eclipse.dash.handbook.git' $ _AFAICT, I was declined before an ECA existence check.
But, this isn't the scenario that I've described.
My expectation is that as a committer, I should at least get some indication whether or not the commit authors in a merge request have signed the ECA. I see no such indication.
The "Merge" button is enabled, but when I click it, it tells me that an error has occurred with no indication of the nature of the error.
Something went wrong during merge pre-receive hook..Is this the ECA checker in action?
AFAICT, I was declined before an ECA existence check.
This is correct sir. This is the Gitlab access permissions playing in. If you create a fork and push to that, you should be able to get the ECA message.
Is this the ECA checker in action?
Yes, it is, but something screwy seems to have happened in Gitlab since the last upgrade as this used to work. There is no indication that this should be broken, so my guess is that something internal changed and this slipped through the cracks at Gitlab. Their documentation page still indicates I am doing this correctly, which I've outlined below as proof/technical details.
See docs: https://docs.gitlab.com/ee/administration/server_hooks.html#custom-error-messages
My code:
- Where I print the message: https://gitlab.eclipse.org/eclipsefdn/it/api/git-eca-rest-api/-/blob/master/src/main/rb/eca.rb#L161
- Where it exits after that message: https://gitlab.eclipse.org/eclipsefdn/it/api/git-eca-rest-api/-/blob/master/src/main/rb/eca.rb#L161
Even if it is erroring out somewhere else, I've pretty thoroughly covered my bases for error checking and used the same mechanism to indicate errors to the UI for each of those cases.
Related issue: #1034 (closed)
The handbook currently says this:
The Eclipse Contributor Agreement (ECA) hook inspects incoming merge requests to ensure that the contributor has a valid ECA on file, and flags those that do not. Project committers should only accept merge requests that pass this validation.
I wrote this based on an understanding that merge requests in GitLab were annotated in a manner similar to what we do with GitHub. But, assuming that I understand what's been discussed here, this is not the case.
Can somebody describe what actually happens from the committer's perspective? Bullet points are fine.
The big differences currently are the following:
- forked repos are skipped atm as there were problems with false negatives and commit newness accuracy that couldn't be easily resolved.
- If the commit does not validate, it blocks the commit rather than notify, which is more inline with the Gerrit way of things. It prints the error messages to the console in a way that the UI should register but it doesn't. If you are pushing directly to the project repo rather than a fork, you should see the messages as normal in your CLI.
@cguindon and I have planned to take a hard look at the GL ECA check and do some work on it as there are new features to be added and improvements to be done. The portal project took a lot of our time and I haven't had the block of time needed to put meaningful effort in until recently.
So as a committer my UX on a merge request is:
- I will see a "Merge" button regardless of the ECA status of authors of the commits;
- An attempt to merge will fail when an author of a commit does not have an ECA.
new features to be added and improvements to be done
If you're compiling a list, a good feature would be to add a comment that indicates the ECA status and (when necessary) gives the merge request's commits' authors help to sign the ECA.
added ECA ~16083 labels
mentioned in issue #1067 (closed)
mentioned in issue #1099 (closed)
added to epic &12 (closed)
mentioned in epic &12 (closed)
added statewip label
The initial version of this is complete and I've passed a message to @droy to update the script at his convenience. The change for this should be that you get a URL in the message that you can go to that links the Git commit hash to the messages. While we need to improve this UI as it isn't super clear, the links won't change!
mentioned in issue #2051 (closed)
