Skip to content

[CI] support sending a PR to each ecs logging agent when spec changes #47

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

Merged
merged 15 commits into from
Feb 5, 2021

Conversation

v1v
Copy link
Member

@v1v v1v commented Feb 1, 2021

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 ecs agents, if and only if there are changes in the spec files. To subscribe an agent, simply add an entry in the .ci/.jenkins-loggers.yml file:
---
loggers:
  - REPO: "ecs-logging-java"
    SPEC_FILEPATH: "ecs-logging-core/src/test/resources/spec/spec.json"
  • Executes a shell script per agent, this script should:
  1. clone the agent repo
  2. ensure the target directory exists
  3. copy the spec.json file 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 ecs agents) whenever a change on the specs is done. This way the repos are always up-to-date with the specs.

Issues

Closes #30
Requires elastic/apm-pipeline-library#936

@apmmachine
Copy link

apmmachine commented Feb 1, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Started by timer

    • Start Time: 2021-02-05T04:03:00.653+0000
  • Duration: 3 min 53 sec

  • Commit: 730c89a

@v1v v1v marked this pull request as ready for review February 2, 2021 16:05
@v1v v1v added the automation label Feb 2, 2021
Copy link
Member

@trentm trentm left a comment

Choose a reason for hiding this comment

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

For now, and probably for the foreseeable future, the ecs-logging.git spec file is just the single "spec.json" file, unlike the apm-server specs.

Also, the two go repos appear to be using a different filename ("v1.json") rather than just using the same "spec.json" file basename. That means this code will need to adjust. Some possible ideas:

  • change "SPEC_PATH" in .jenkins-loggers.yml to be the full file path, e.g.:
...
  - REPO: "ecs-dotnet"
    SPEC_PATH: "tests/Elastic.CommonSchema.Tests/Specs/spec.json"
  - REPO: "ecs-logging-go-logrus"
    SPEC_PATH: "internal/spec/v1.json"
...
  • Consider dropping the filesType variables. I think it is unlikely that anything other than spec.json will be relevant for the foreseeable future.
  • Consider changing filesPath to fileName or something to indicate it is just the single file.

v1v and others added 3 commits February 2, 2021 16:43
Co-authored-by: Trent Mick <trentm@gmail.com>
Co-authored-by: Trent Mick <trentm@gmail.com>
Co-authored-by: Trent Mick <trentm@gmail.com>
@axw
Copy link
Member

axw commented Feb 3, 2021

Also, the two go repos appear to be using a different filename ("v1.json") rather than just using the same "spec.json" file basename. That means this code will need to adjust. Some possible ideas:

If it's less work overall, I think we could also just rename v1.json to spec.json in the Go repos. @simitt any concerns about that?

@v1v
Copy link
Member Author

v1v commented Feb 3, 2021

I just added support to different names with c8205f5 we can change if any repo changes the name of those spec.json files

@v1v v1v requested a review from trentm February 3, 2021 11:06
Copy link
Member

@trentm trentm left a comment

Choose a reason for hiding this comment

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

Have you been able to test this? I know it is probably hard to do so realistically.

git config user.email
git checkout -b "update-spec-files-$(date "+%Y%m%d%H%M%S")"
git add "${SPECS_FILEPATH}"
git commit -m "test: synchronizing specs"
Copy link
Member

Choose a reason for hiding this comment

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

Is this a commit message that is wanted?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the commit in the PR, therefore the person who squash and merge could potentially change the title of the PR. Since there was no specifications at the time of writing this, but using what it was already implemented for other similar projects.

I guess, this is something that can be potentially changed in the future with the message that the apm agents have agreed, do you think for the time being we can move forward?

Copy link
Member

Choose a reason for hiding this comment

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

do you think for the time being we can move forward?

Yup, definitely. Thanks.

@v1v v1v merged commit 8db8fe8 into elastic:master Feb 5, 2021
@v1v v1v deleted the feature/ecs-logging branch February 5, 2021 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sync spec to logger repos
6 participants