-
Notifications
You must be signed in to change notification settings - Fork 20
[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
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
|
There was a problem hiding this 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
tofileName
or something to indicate it is just the single file.
Co-authored-by: Trent Mick <trentm@gmail.com>
Co-authored-by: Trent Mick <trentm@gmail.com>
Co-authored-by: Trent Mick <trentm@gmail.com>
If it's less work overall, I think we could also just rename |
I just added support to different names with c8205f5 we can change if any repo changes the name of those spec.json files |
There was a problem hiding this 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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
What is this PR doing?
It adds a Jenkins pipeline executing on pushes to the master branch, with the following stages:
.ci/.jenkins-loggers.yml
file:spec.json
file to the target directoryWhy 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