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

[JENKINS-53707] Allow commit message and author of head commit #145

Merged
merged 1 commit into from
Oct 30, 2018
Merged

[JENKINS-53707] Allow commit message and author of head commit #145

merged 1 commit into from
Oct 30, 2018

Conversation

nfalco79
Copy link
Member

All the data gather from BB (cloud or server) lacks of message and author for HEAD commit. Actually only commit hash is stored, instead commit message and author are ignored.
The PR simply add methods to get move on those information.
As explained in the JIRA issue the goal is avoid trigger a build if the last commit of a branch is performed by the Jenkins user or if the commit message matches some rules (for example [maven-release-plugin]*). This is useful for example when in the pipeline we perform some git push (for example during release).
The author is in git commit raw format, that mean author name <author email> for both client.

Test cases does not use the actual mock api client because during test I got a some NPE in JSON de-serialisation that can be captured only using real REST calls. For this scope I had create a new integration api client that rely on real payloads get from this sample repository (replicated also on BB server 5.14.0 with a trial license). Each call is translated to a json file on classpath. The client could also take a different root path where place payloads from a different sample repository site.

Eventually the PR could be split into 2 part. The main part is expose message/author information, the second one (optional) is the CommitAuthorFilterTraits. This traits could be also implemented as additional jenkins plugin but the first part of this PR is required.

@witokondoria
Copy link
Member

From previous talks with @stephenc, traits inside this plugin were supposed to be kept to the minimum. As an outcome, a trait to filter by commit messages was (partially implemented) https://github.com/jenkinsci/scm-trait-commit-skip-plugin. Maybe this trait implementation could/shold be placed there?

@nfalco79
Copy link
Member Author

Yes could be, as I wrote the PR can be split, but the main part is expose commit information. Before split I would someone review these changes and if they feel to move out the trait than I will remove it.

Copy link
Member

@stephenc stephenc left a comment

Choose a reason for hiding this comment

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

Needs splitting

@nfalco79
Copy link
Member Author

nfalco79 commented Oct 1, 2018

Split done

@jetersen
Copy link
Member

jetersen commented Oct 7, 2018

@nfalco79 I would like a link to the plugin going to implement this wanted feature 😅
Hoping that you would work together with @witokondoria 🙌

@nfalco79
Copy link
Member Author

nfalco79 commented Oct 7, 2018

Yes, when this will be merged I will propose the other part of this PR at the @witokondoria repo. Untill merged i can not propose a PR that compile

@jetersen
Copy link
Member

jetersen commented Oct 7, 2018

You can! That is why we have incrementals!

If check the 2 successful checks:
image

The details link will take you to a incremental version of your PR.
https://repo.jenkins-ci.org/incrementals/org/jenkins-ci/plugins/cloudbees-bitbucket-branch-source/2.2.13-rc440.a1f1e6c1d925/

Is that not just wonderful @nfalco79 ? 🙌🙌🙌

@jetersen
Copy link
Member

jetersen commented Oct 7, 2018

You just update the POM, for the bitbucket branch source dependency version to the incremental version number!

I would not want to merge this before I can see the counter part working, would I? So your work would be downstream of this PR.

@witokondoria
Copy link
Member

In fact, I'm working on new traits using this implemented feature. Including a list of strings instead of fixed values ([ci skip] and [skip ci]) will be trivial

@nfalco79
Copy link
Member Author

nfalco79 commented Oct 7, 2018

You just update the POM, for the bitbucket branch source dependency version to the incremental version number!

@Casz good to know

I would not want to merge this before I can see the counter part working, would I?

I'm not agree, implemented test cases are used for this specific goal. Code written against these changes at 99% will be mocked so it would have little to do with logic written here expect for the interfaces.

@stephenc
Copy link
Member

stephenc commented Oct 7, 2018

If you don’t show how the api you are adding will be used for real then it is harder to review. In general we wait to see a concrete usage before agreeing to merge

@nfalco79
Copy link
Member Author

nfalco79 commented Oct 7, 2018

I understand but here I had expose only existing data, I'm not adding new APIs with a javadoc that describes what an implementation should do without a default implementation.

Anyway I'm working to propose trait code on scm-trait-commit-skip-plugin project

@nfalco79
Copy link
Member Author

nfalco79 commented Oct 7, 2018

Submitted a PR jenkinsci/scm-trait-commit-skip-plugin#10 with the example trait (it's similar to our company trait plugin to avoid trigger 3 builds at release time).

@nfalco79
Copy link
Member Author

The PR is reworked a bit to support commit information on pull request and to revision. Now there are revision specific for bitbucket (and tag). The reason to decorate bitbucket revision with these commit informations is to provide them to BranchBuildStrategy. Now it's easy implements something to archive JENKINS-45502

@nfalco79
Copy link
Member Author

nfalco79 commented Oct 17, 2018

I have update examples on jenkinsci/scm-trait-commit-skip-plugin#10

There are two example of use of commit information:

  1. a trait that discard branch where the head commit (date) is older than selected days (for example discard and free space on slaves if branch has not new commit since 1 year)
    EDIT available here
  2. a build strategy that do not trigger a build if the head commit of a branch (from webhook or indexing) matches the author or message

@jetersen
Copy link
Member

@nfalco79 build is red 😇

@xmj
Copy link

xmj commented Oct 19, 2018

Now it's not :)

@jetersen
Copy link
Member

I am going to Nice for Jenkins world so won't be able to until Friday or if I get time to it en the evening 😊

this.author = commit.getAuthor();
Date commitDate;
try {
commitDate = new BitbucketDateFormat().parse(commit.getDate());

Choose a reason for hiding this comment

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

I am interested in your feature of traits for skipping CI builds so I built a private copy of this PR as well as the corresponding one in the scm-trait-commit-skip-plugin. We use Bitbucket 4.5 in our CI and this line causes NPE when running this plugin. I added a null check for commit.getDate() and fixed the problem.

Would you make this change so the feature also works for older versions of Bitbucket?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only reference I can find about the Server version is 5.2.0 in the README.MD file.

I could protect against NPE here but this does not prevent NPE for who use this field because commit date is assumed be always available or trait logic could not work.

Maybe the field was named different in version 4.5 so could you please report the REST payload for your server version?

REST paths for which provide payload (you can clone this public repo so I could compare against payload of test case in this PR) are:

  1. https://<server>:<port>/<server_context>/1.0/projects/<owner>/repos/<repo_name>/branches?start=0&limit=200
  2. https://<server>:<port>/<server_context>/1.0/projects/<owner>/repos/<repo_name>/commits/046d9a3c1532acf4cf08fe93235c00e4d673c1d2

Choose a reason for hiding this comment

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

You have a very good point. We are planning a server upgrade in the next couple of months so I think I will just workaround this when you finalize your changes until we have the latest Bitbucket server running. I know I am pushing the limits of compatibility here using such an old Bitbucket server with the latest Jenkins.

For your reference the payload from the 4.5 server with the above REST queries is this after I fed it to jq . so it is human readable:

{
  "start": 0,
  "values": [
    {
      "isDefault": false,
      "latestChangeset": "bf0e8b7962c024026ad01ae09d3a11732e26c0d4",
      "latestCommit": "bf0e8b7962c024026ad01ae09d3a11732e26c0d4",
      "type": "BRANCH",
      "displayId": "release/release-1",
      "id": "refs/heads/release/release-1"
    },
    {
      "isDefault": false,
      "latestChangeset": "046d9a3c1532acf4cf08fe93235c00e4d673c1d2",
      "latestCommit": "046d9a3c1532acf4cf08fe93235c00e4d673c1d2",
      "type": "BRANCH",
      "displayId": "feature/BB-2",
      "id": "refs/heads/feature/BB-2"
    },
    {
      "isDefault": false,
      "latestChangeset": "fb522a6f08c7c7df337312e4e65ec1b57710672e",
      "latestCommit": "fb522a6f08c7c7df337312e4e65ec1b57710672e",
      "type": "BRANCH",
      "displayId": "feature/BB-1",
      "id": "refs/heads/feature/BB-1"
    },
    {
      "isDefault": true,
      "latestChangeset": "bf4f4ce8a3a8d5c7dbfe7d609973a81a6c6664cf",
      "latestCommit": "bf4f4ce8a3a8d5c7dbfe7d609973a81a6c6664cf",
      "type": "BRANCH",
      "displayId": "master",
      "id": "refs/heads/master"
    }
  ],
  "isLastPage": true,
  "limit": 25,
  "size": 4
}

Raw:

{"size":4,"limit":25,"isLastPage":true,"values":[{"id":"refs/heads/release/release-1","displayId":"release/release-1","type":"BRANCH","latestCommit":"bf0e8b7962c024026ad01ae09d3a11732e26c0d4","latestChangeset":"bf0e8b7962c024026ad01ae09d3a11732e26c0d4","isDefault":false},{"id":"refs/heads/feature/BB-2","displayId":"feature/BB-2","type":"BRANCH","latestCommit":"046d9a3c1532acf4cf08fe93235c00e4d673c1d2","latestChangeset":"046d9a3c1532acf4cf08fe93235c00e4d673c1d2","isDefault":false},{"id":"refs/heads/feature/BB-1","displayId":"feature/BB-1","type":"BRANCH","latestCommit":"fb522a6f08c7c7df337312e4e65ec1b57710672e","latestChangeset":"fb522a6f08c7c7df337312e4e65ec1b57710672e","isDefault":false},{"id":"refs/heads/master","displayId":"master","type":"BRANCH","latestCommit":"bf4f4ce8a3a8d5c7dbfe7d609973a81a6c6664cf","latestChangeset":"bf4f4ce8a3a8d5c7dbfe7d609973a81a6c6664cf","isDefault":true}],"start":0}

And

{
  "parents": [
    {
      "parents": [
        {
          "displayId": "8d0fa145bde",
          "id": "8d0fa145bde5151f1d103ab1c3dc1033e6ec4ac1"
        }
      ],
      "message": "Add sample script hello world",
      "authorTimestamp": 1537538845000,
      "author": {
        "emailAddress": "amuniz@example.com",
        "name": "Antonio Muniz"
      },
      "displayId": "bf4f4ce8a3a",
      "id": "bf4f4ce8a3a8d5c7dbfe7d609973a81a6c6664cf"
    }
  ],
  "message": "Add one message more",
  "authorTimestamp": 1537541363000,
  "author": {
    "emailAddress": "nfalco79@acme.com",
    "name": "Nikolas Falco"
  },
  "displayId": "046d9a3c153",
  "id": "046d9a3c1532acf4cf08fe93235c00e4d673c1d2"
}

Raw:

{"id":"046d9a3c1532acf4cf08fe93235c00e4d673c1d2","displayId":"046d9a3c153","author":{"name":"Nikolas Falco","emailAddress":"nfalco79@acme.com"},"authorTimestamp":1537541363000,"message":"Add one message more","parents":[{"id":"bf4f4ce8a3a8d5c7dbfe7d609973a81a6c6664cf","displayId":"bf4f4ce8a3a","author":{"name":"Antonio Muniz","emailAddress":"amuniz@example.com"},"authorTimestamp":1537538845000,"message":"Add sample script hello world","parents":[{"id":"8d0fa145bde5151f1d103ab1c3dc1033e6ec4ac1","displayId":"8d0fa145bde"}]}]}

Copy link
Member Author

@nfalco79 nfalco79 Oct 27, 2018

Choose a reason for hiding this comment

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

I had compare and the cause is not due the different server version. So maybe you get a bug, could you post the full stacktrace of the NPE to point me on the right direction?

Please confirm on which commit is based your bitbucket-branch-source-plugin installed. Could you install this HPI ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could I think you have a previous commit where there was used the repo.getUpdateOn that was always null?

Choose a reason for hiding this comment

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

Sorry for the late response. I can try with the merged master branch now and see what the result is.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

please change your project settings for imports to: no grouping of imports A-Z non static before static

Do not use the javax.annotation for now use the findbugs annotation

@nfalco79
Copy link
Member Author

@Casz changed as request

Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

Found a few more 😇

@nfalco79
Copy link
Member Author

Double checked with text search in Changes tab

Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

Last one I hope 😅

Value correctly message for both implementation of cloud and server. Add author to the BitbucketCommit interface, it is valued using the typical raw git representation "name <email>". Add commit information for pullrequest and revision to allow logic on
build strategy
@nfalco79
Copy link
Member Author

Fix javadoc, rebase and squash

@jetersen
Copy link
Member

Last one: src\test\resources\com\cloudbees\jenkins\plugins\bitbucket\client\payload\2.0-repositories-amuniz-test-repos-pullrequests-1-commits_fields_values.hash_2Cvalues.author.raw_2Cvalues.date_2Cvalues.message_pagelen_1.json file name is too long on my poor windows machine, I can of course figure out how to enable long paths... Which I thought I did.

@nfalco79
Copy link
Member Author

nfalco79 commented Oct 29, 2018

Last one: src\test\resources\com\cloudbees\jenkins\plugins\bitbucket\client\payload\2.0-repositories-amuniz-test-repos-pullrequests-1-commits_fields_values.hash_2Cvalues.author.raw_2Cvalues.date_2Cvalues.message_pagelen_1.json file name is too long on my poor windows machine, I can of course figure out how to enable long paths... Which I thought I did.

relocate the git repository or use the git config --system core.longpaths true to allow git go over windows 260 chars limitation. (in unix is 4096)

EDIT: Anyway at work (where I develop this PR) I had use windows without problems. I will try to checkout the branch from scratch. (I use smartgit as git client)

@jetersen
Copy link
Member

jetersen commented Oct 29, 2018

GitKraken apparently doesn't respect long paths, so I had to go to the git cli 😅

@nfalco79
Copy link
Member Author

nfalco79 commented Oct 29, 2018

It's ok because at work I do not exceed the 260 chars. For similar issues in the past we usually have the root git folder as short path as possible (D:\git<repos>).

@jetersen jetersen merged commit e5e02fc into jenkinsci:master Oct 30, 2018
@nfalco79 nfalco79 deleted the feature/JENKINS-53707 branch November 4, 2018 21:51
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.

7 participants