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

Update remotecfg dep to use alloy-remote-config over agent-remote-config #748

Merged
merged 11 commits into from May 13, 2024

Conversation

erikbaranowski
Copy link
Contributor

@erikbaranowski erikbaranowski commented May 2, 2024

PR Description

Closes #783

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>
Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>
Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>
Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>
@erikbaranowski erikbaranowski marked this pull request as ready for review May 2, 2024 19:26
Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

Generally OK with these changes. I'm a little worried that it might be an abrupt change even if we can technically make these kinds of changes for non-stable functionality. Do we have a pulse on how many people this might impact, and if we need to consider a smoother migration?

CHANGELOG.md Outdated
@@ -10,6 +10,11 @@ internal API changes are not present.
Main (unreleased)
-----------------

### Breaking changes
Copy link
Member

Choose a reason for hiding this comment

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

@clayton-cornell Should we find a different way of classifying these kinds of changes for non-stable functionality now that we're post-1.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe append something like this to the header? Breaking changes (Public preview features)

Copy link
Member

Choose a reason for hiding this comment

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

cc @richardqlam on this too

Choose a reason for hiding this comment

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

Hm I'm not sure further sub-categorizing breaking changes at a top level is all that useful. Feels like a changelog is most consumable when it talks about things on a feature-by-feature level. And then within the context of each feature highlighting what state it is (GA, public preview, etc).

Copy link
Contributor

Choose a reason for hiding this comment

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

Just looking at this now. It's still a breaking change, even though it's in a public preview state. The info clearly says it's an update to the public preview.... to my way of thinking, it's clear enough as-is.

go.mod Outdated Show resolved Hide resolved
Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>
@erikbaranowski
Copy link
Contributor Author

@rfratto As far as usage we don't have a great pulse other than a couple questions about remotecfg in community slack we have received and looking at the number of watchers/stars in the following repos. We have combined 1 star and 5 watchers each right now so I do feel this is not impacting a lot of folks.

@rfratto rfratto added the backport-to-agent:no PR should NOT be backported to the agent repo. label May 3, 2024
Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>
Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>
@erikbaranowski erikbaranowski enabled auto-merge (squash) May 13, 2024 19:42
@erikbaranowski erikbaranowski merged commit 6a195d6 into main May 13, 2024
18 checks passed
@erikbaranowski erikbaranowski deleted the update-remotecfg-dep branch May 13, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-agent:no PR should NOT be backported to the agent repo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update remotecfg dep to use alloy-remote-config over agent-remote-config
4 participants