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

updatecli: use shared policy #4121

Merged
merged 3 commits into from
Jul 9, 2024
Merged

updatecli: use shared policy #4121

merged 3 commits into from
Jul 9, 2024

Conversation

v1v
Copy link
Member

@v1v v1v commented Jul 5, 2024

What does this PR do?

Use a shared updatecli policy. The exact configuration is used in different GitHub repositories.

I ran https://github.com/elastic/apm-agent-nodejs/actions/runs/9806099782. And it created the PR:

#4122 vs #4046

Checklist

  • Implement code
  • Add tests
  • Update TypeScript typings
  • Update documentation
  • Add CHANGELOG.asciidoc entry
  • Commit message follows commit guidelines

@v1v v1v self-assigned this Jul 5, 2024
@v1v v1v marked this pull request as ready for review July 5, 2024 09:24
@v1v v1v requested review from a team July 5, 2024 09:24
reakaleek
reakaleek previously approved these changes Jul 5, 2024
trentm
trentm previously approved these changes Jul 5, 2024
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.

Approved with some reservations. The end result here feels like a very complex solution to the problem of periodically checking for updates to some files vendored from another repo.

update-compose.yaml Show resolved Hide resolved
@@ -0,0 +1,12 @@
policies:
- name: Handle apm-data server specs
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately these "name:" Handle ... strings do not appear in the workflow run log at all, e.g.: https://github.com/elastic/apm-agent-nodejs/actions/runs/9806099782/job/27077125378

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll raise an issue in the upstream project so it can be honoured or at least they know this case. Thanks

@@ -0,0 +1,12 @@
policies:
- name: Handle apm-data server specs
policy: ghcr.io/elastic/oblt-updatecli-policies/apm/apm-data-spec:0.2.0@sha256:7069c0773d44a74c4c8103b4d9957b468f66081ee9d677238072fe11c4d2197c
Copy link
Member

Choose a reason for hiding this comment

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

These policy files are behind credentials that I don't have, so are uninspectable by me. Do these live in a repo somewhere that I could look at? Here I guess:
https://github.com/elastic/oblt-updatecli-policies/tree/main/updatecli/policies/apm/apm-data-spec

The "0.2.0" container image label doesn't have a matching git tag in that repository. I see there is a changelog, but it is a bit of a guess which commit a particular container image build corresponds to.

Perhaps at least a clickable URL to be able to go look at the related code? Something like:

Suggested change
policy: ghcr.io/elastic/oblt-updatecli-policies/apm/apm-data-spec:0.2.0@sha256:7069c0773d44a74c4c8103b4d9957b468f66081ee9d677238072fe11c4d2197c
# https://github.com/elastic/oblt-updatecli-policies/tree/main/updatecli/policies/apm/apm-data-spec
policy: ghcr.io/elastic/oblt-updatecli-policies/apm/apm-data-spec:0.2.0@sha256:7069c0773d44a74c4c8103b4d9957b468f66081ee9d677238072fe11c4d2197c

Though, that is of limited help, because it is very difficult to understand what this workflow is attempting to do and to follow what the policy is doing. I can make guesses, but for example, it isn't clear to me what build process in oblt-updatecli-policies is rendering the https://github.com/elastic/oblt-updatecli-policies/blob/main/updatecli/policies/apm/apm-data-spec/updatecli.d/default.tpl template to a useable final thing that updatecli eventually executes.

Copy link
Member Author

@v1v v1v Jul 8, 2024

Choose a reason for hiding this comment

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

These policy files are behind credentials that I don't have

Not at all, you can login to GitHub itself:

Do these live in a repo somewhere that I could look at?

I didn't want to explicitly write the GitHub repository, it's private at the moment, I'm waiting for an approval to set it public available.

elastic/oblt-updatecli-policies@main/updatecli/policies/apm/apm-data-spec

correct

Perhaps at least a clickable URL to be able to go look at the related code? Something like:

No, I plan to use updatecli to bump the versions automatically; see elastic/apm-agent-go#1633

I'll raise an issue to ask to the upstream project what's the strategy about the changelog automation for those automated-bumps touching the updatecli policies.

Copy link
Member Author

Choose a reason for hiding this comment

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

Though, that is of limited help, because it is very difficult to understand what this workflow is attempting to do and to follow what the policy is doing.

Do you know how I can help with this? I'm open to making this better.

The idea behind the updatecli shareable policies is to share updatecli pipelines between different GitHub projects and then use 'values' to configure the behaviour. That's why the 'default.tpl' exists and then infer the values.

In this particular case, these changes are all related to syncing up the apm and apm-data specs/gherkins in all the APM Agents, see https://github.com/elastic/observability-robots/issues/1751 (Only for Elastic employees)

@v1v
Copy link
Member Author

v1v commented Jul 8, 2024

The end result here feels like a very complex solution to the problem of periodically checking for updates to some files vendored from another repo.

I'm not sure what's the reason for the complex solution:

  • Configure the values in specific YAML files (one per policy)
  • Configure the shared policies in the root file update-compose.yaml
  • GitHub action to configure the environment (updatecli, credentials) and run the updatecli policies.

It's idempotence and will create Pull Requests with changes if needed.

All the above is required in 8 or 9 different GitHub projects. I already went through changing behaviour in the updatecli pipeline, and had to deal with several PRs.

Using a centralised shared place for well defined policies will help us by different means:

  • easy to onboard new projects
  • reduce duplication
  • tests those polices in isolation.
  • version them

The very end goal is to use automation, therefore if a new version is released for the updatecli policies, the file update-compose.yaml will be automatically updated itself, see elastic/apm-agent-go#1634

Please let me know if I need to clarify anything, I'll raise a few GitHub issues in the updatecli project based on your valuable feedback, 🙏

@trentm
Copy link
Member

trentm commented Jul 8, 2024

Please let me know if I need to clarify anything

All good. Thanks for feedback.

Perhaps add that comment at the top of "update-compose.yaml" stating that the file is related to updatecli? If that is a pain because you need this to be common across the many apm-agent repos, then you can skip it. Thanks.

Note to self: I can browse the published ghcr.io packages here:
https://github.com/orgs/elastic/packages?tab=packages&q=oblt
https://github.com/elastic/oblt-updatecli-policies/pkgs/container/oblt-updatecli-policies%2Fapm%2Fapm-data-spec

Co-authored-by: Trent Mick <trent.mick@elastic.co>
@v1v v1v dismissed stale reviews from trentm and reakaleek via 5981a5e July 9, 2024 06:24
@v1v
Copy link
Member Author

v1v commented Jul 9, 2024

Perhaps add that comment at the top of "update-compose.yaml" stating that the file is related to updatecli?

Not at all, see 5981a5e (#4121)

@v1v v1v requested review from trentm and reakaleek July 9, 2024 06:25
@v1v v1v merged commit 70975da into main Jul 9, 2024
19 checks passed
@v1v v1v deleted the feature/use-shared-policies branch July 9, 2024 17:07
trentm pushed a commit that referenced this pull request Jul 24, 2024
fpm-peter pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
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.

3 participants