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

Adding support the new Gitlab Pull Request Plugin #134

Closed
wants to merge 7 commits into from

Conversation

ashwanthkumar
Copy link
Collaborator

@ashwanthkumar ashwanthkumar commented Oct 23, 2020

We now have Gitlab Merge Requests Plugin for GoCD and it is identified by gitlab.pr. This change extends the existing GitLab notifications to support gitlab.pr scm plugin as well.

The existing setup allowed only a single pluginId for the poller, updated that to support multiple values.
this is strictly not equivalent, but given the source of the data doesn't change this is fine
@kritika-singh3
Copy link
Contributor

kritika-singh3 commented Oct 27, 2020

@ashwanthkumar Thanks for your contribution!
We tested the PR build with both git.fb and gitlab.mr materials.
Results using gitlab:

  • With git.fb material, status was updated successfully
  • With gitlab.mr material, no status updated

In both the cases, the logs mentioned sending the status update but in only the former case,it was shown in the UI.
Can you please take a look and fix it?

Map materialConfiguration = (Map) material.get("scm-configuration");
String url = (String) materialConfiguration.get("url");

List<Map> modifications = (List<Map>) materialRevision.get("modifications");
String revision = (String) modifications.get(0).get("revision");
Map modificationData = (Map) modifications.get(0).get("data");
String prId = (String) modificationData.get("PR_ID");
String prBranch = (String) modificationData.getOrDefault("PR_BRANCH", "PR_ID");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Realised we don't use this field of updateStatus for any other provider except for Gitlab. Earlier implementation was pushing the commit status as part of the merge-request refspec instead of the source branch. That's the reason why the commit status wouldn't show up on the Merge Request page.

@ashwanthkumar
Copy link
Collaborator Author

@kritika-singh3 Thanks for the feedback, the commit status was not coming up on the Merge Request page earlier. I thought it was some misconfiguration on our Gitlab deployment 😅 . It is fixed now though.

@kritika-singh3
Copy link
Contributor

kritika-singh3 commented Oct 27, 2020

@ashwanthkumar Thanks!! Will verify the same and let you know soon!!

@kritika-singh3
Copy link
Contributor

@ashwanthkumar We are still unable to see the status:

Screenshot 2020-10-28 at 10 43 14 AM

Can you verify and share the details (e.g. GoCD version, Java version) of your test cases?

@ashwanthkumar
Copy link
Collaborator Author

@kritika-singh3 I tested these changes on GoCD 19.12 running JVM 11.0. A really stupid question though, can you confirm and tell me what version of the plugin jar you're testing against the latest changes? I was initially testing things against 1.6-68 and realised I pulled in latest changes from master so the version is now 1.6-106. It took me 1 solid hour to figure out why something wasn't working as expected 😅

@kritika-singh3
Copy link
Contributor

@ashwanthkumar I was testing the changes on latest GoCD master (i.e. on an experimental GoCD 20.9.0-12323) with JVM 13. My plugin version is 1.6-106. I built the plugin post pulling the PR branch itself.

Let me give it a try using the GoCD version 19.12. I'll keep you posted.

@kritika-singh3
Copy link
Contributor

@ashwanthkumar I tested the build on clean new GoCD 19.12.0 setup with JVM 11.0.2 (bundled one). I used the following configurations:

Cruise config
<scms>
    <scm id="aaaee3ff-0095-46a6-998f-a25dfebb216b" name="gitlab">
      <pluginConfiguration id="gitlab.pr" version="1" />
      <configuration>
        <property>
          <key>url</key>
          <value>https://gitlab.com/testing-group12/public-repo</value>
        </property>
        <property>
          <key>defaultBranch</key>
          <value>master</value>
        </property>
      </configuration>
    </scm>
    <scm id="6f7c7468-df9c-453a-9293-02caa452c4ce" name="git.fb-one">
      <pluginConfiguration id="git.fb" version="1" />
      <configuration>
        <property>
          <key>url</key>
          <value>https://gitlab.com/testing-group12/public-repo</value>
        </property>
        <property>
          <key>defaultBranch</key>
          <value>master</value>
        </property>
      </configuration>
    </scm>
  </scms>
<pipelines group="defaultGroup">
    <pipeline name="test">
      <materials>
        <scm ref="aaaee3ff-0095-46a6-998f-a25dfebb216b" />
      </materials>
      <stage name="stage">
        <jobs>
          <job name="job" timeout="8">
            <tasks>
              <exec command="ls" />
              <exec command="env" />
            </tasks>
          </job>
        </jobs>
      </stage>
    </pipeline>
    <pipeline name="git-test">
      <materials>
        <scm ref="6f7c7468-df9c-453a-9293-02caa452c4ce" />
      </materials>
      <stage name="stage">
        <jobs>
          <job name="job" timeout="8">
            <tasks>
              <exec command="ls" />
              <exec command="env" />
            </tasks>
          </job>
        </jobs>
      </stage>
    </pipeline>
  </pipelines>

Plugin Details:

  • Git Feature Branch plugin Version: 1.4.0-RC4
  • Gitlab Pull Requests Builder Version: 1.4.0-RC4
  • GitLab Merge Requests status notifier Version: 1.6-106

Plugin settings:

Screen Shot 2020-10-30 at 11 27 48 AM

Note: My token has api, read_repository scope.

The status was represented only for git-test pipeline

Screen Shot 2020-10-30 at 11 27 02 AM

@chadlwilson
Copy link
Contributor

I've rebased this into #249 but I suspect the comment in #134 (comment) has not been resolved, so it may still not be working. At least folks can sanity test it with the experimental build there....

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