Jenkins pipeline for sending PRs to agents when changing the BDD specs

[mdelapenya]

What is this PR doing?

It adds a Jenkins pipeline executing on pushes to the master branch, with the following stages:

• Checkouts the SCM
• Uses a parallel execution to send PRs to each subscribed agent, if and only if there are changes in the gherkin files. To subscribe an agent, simply add an entry in the .ci/.jenkins-agents.yml file:

---
agents:
  - NAME: "python"
    FEATURES_PATH: "tests/bdd/features"
  - NAME: "ruby"
    FEATURES_PATH: "features"

The name will represent the agent name (equals to the github repository: i.e. "apm-agent-python"), and the features path, the directory in the agent repository where the feature files are located.

• Installs HUB in the worker
• Executes a shell script per agent, this script should:

1. clone the agent repo
2. ensure the target directory exists
3. copy the feature files to the target directory
4. create a git commit with the changes
5. send a PR to the default branch of the agent repo with the changes

Why is important?

We want to push changes to downstream repos (the agents) whenever a change on the specs is done. This way the repos are always up-to-date with the specs.

Follow-ups

• Consider versioning the specs, so each agent follows a spec.
• Consider destructive changes in the specs (deletion): they won't be applied as we are just copying the files. We could remove the target dir first, and then apply the shared specs.

[SylvainJuge]

For cross-agent features, we create a check list to make sure that we don't forget to update each agent, maybe it would be interesting to do the same here and test this for all agents. example: #92 (comment)

Also, while this "automatic PR on merged specs" will definitely be useful, we need to be able to update each agent specs independently (as we currently do with scripts similar to https://github.com/elastic/apm-agent-java/blob/master/scripts/shared-specs/download_gherkins.sh ), as it allows to work on not-yet-merged specs.

[mdelapenya]

For cross-agent features, we create a check list to make sure that we don't forget to update each agent, maybe it would be interesting to do the same here and test this for all agents. example: #92 (comment)

Also, while this "automatic PR on merged specs" will definitely be useful, we need to be able to update each agent specs independently (as we currently do with scripts similar to https://github.com/elastic/apm-agent-java/blob/master/scripts/shared-specs/download_gherkins.sh ), as it allows to work on not-yet-merged specs.

Hey @SylvainJuge, could those custom agent specs be done in a different file? If each agent needs to contribute custom scenarios to a feature file, then I would suggest having a code-generation process that takes the upstream file as template, and then applies agent contributions.

We can discuss about that in a follow-up iteration. Does it make sense to you?

[SylvainJuge]

Hey @SylvainJuge, could those custom agent specs be done in a different file? If each agent needs to contribute custom scenarios to a feature file, then I would suggest having a code-generation process that takes the upstream file as template, and then applies agent contributions.

Actually that won't be custom scenarios per agent, if we had such scenarios, we will just put them in a different folder. I was more thinking about one or two agents will have to work with not-yet-merged versions of those shared specs.

My point here is that we will still need to be able to use a different version of those specs in each agent repository, thus it won't completely replace update scripts that currently have.

[mdelapenya]

Ok, got it. In that direction, then I'd suggest discarding the automated PR until the upstream specs are good-to-go. At the end of the day, the PR is not automatically merged 😃

[v1v]

Ideally, the git user.name and user.email should be in place (see PR 17214 in the infra repo), if that's not the case, then you might get some warnings, or maybe some errors, although I don't recall now.

I'd say to use some validation in case they are not defined to set them. what do you think?

[mdelapenya]

Instead of a validation, I have added two gitCmd calls to ensure they are present in the build agent (see 08f9d9a). wdyt?

[mdelapenya]

I started with the check, but I realised that it is worth it to be prescriptive in this case, where we do not want other configuration. Just in case, that was my initial validation, at script level:

readonly GIT_USERNAME="elasticmachine"
readonly GIT_EMAIL="infra-root-${GIT_USERNAME}@elastic.co"

function checkGitConfig {
    local config="${1}"
    local defaultValue="${2}"

    git config "${config}"
    local exitCode=$?
    if [ ${exitCode} -ne 0 ]; then
        git config "${config}" "${defaultValue}"
    fi
}

function checkGit {
    checkGitConfig "user.name" "${GIT_USERNAME}"
    checkGitConfig "user.email" "${GIT_EMAIL}"
}

[v1v]

Yep, PR titles are also linted in some cases

…

On Thu, 26 Mar 2020, 18:59 Manuel de la Peña, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In .ci/scripts/send-pr.sh <#231 (comment)>: > +cp tests/agents/gherkin-specs/*.feature "${APM_AGENT_REPO_DIR}/${APM_AGENT_SPECS_DIR}" + +cd ${APM_AGENT_REPO_DIR} +git checkout -b update-feature-files-$(date "+%Y%m%d%H%M%S") +echo "Copying feature files to the ${APM_AGENT_REPO_NAME} repo" +git status +git add ${APM_AGENT_SPECS_DIR} +git commit -m "ci: Synchronized BDD specs" + +if [[ "${DO_SEND_PR}" == "true" ]]; then + hub pull-request \ + -p \ # push the branch to the remote + -b master \ # target branch + --labels automation \ # comma-separated list of tags + --reviewer @elastic/apm-agents \ # set agents as reviewer of the PR + -m ":robot: Synchronizing BDD specs" # PR message I think this is the message for the PR, not the commit. Is the commitlint checking this body? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#231 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAV5D2SJTLLJZ3PE2R5Z5LDRJOJXFANCNFSM4LTZYFMA> .

[mdelapenya]

This is the block that controls if there are changes in the gherkin files. If so, the environment variable will be set to true, causing https://github.com/elastic/apm/pull/231/files#diff-8db37e8fcea0f1a8f2f39667e94ebcc4R76 to evaluate to true too

[mdelapenya]

Because of this, the script sending the PR has to add no logic for checking if changes exist, so it just follow orders from the pipeline.

[mdelapenya]

Hey @elastic/apm-agent-devs, we would like to know if this PR could be merged. Please feel free to add any comment.