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 elastic-agent-client and rename control proto package to cproto #39586

Merged
merged 4 commits into from
May 24, 2024

Conversation

michel-laterman
Copy link
Contributor

@michel-laterman michel-laterman commented May 15, 2024

Proposed commit message

cherry-pick the commit to update the elastic-agent-client to a more recent version. The PR was reverted because the resulting binary failed to start due to a proto namespace collision.

Rename control proto package to cproto so it does not conflict with elastic-agent-client import

Upgrade integration test found that we had an error when upgrading from the latest 7.17.x snapshots.
Investigation found that while the binary could compile and all tests pass, attempting to run it would cause a panic when the imports executed their init functions as the protobuf namespaces collided.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

compile the agent binary and attempt to run any command, i.e., elastic-agent version

Related issues

@michel-laterman michel-laterman added bug Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team labels May 15, 2024
@michel-laterman michel-laterman requested a review from a team as a code owner May 15, 2024 22:46
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels May 15, 2024
@michel-laterman michel-laterman added backport needs_team Indicates that the issue/PR needs a Team:* label labels May 15, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label May 15, 2024

option cc_enable_arenas = true;
option go_package = "pkg/agent/control/proto;proto";
option go_package = "pkg/agent/control/cproto;cproto";
Copy link
Member

Choose a reason for hiding this comment

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

Can you still upgrade 7.17.x->7.17.x with this change?

In the 8.x agent we maintain two copies of the file but I can't remember if the concern there applies here as well. https://github.com/elastic/elastic-agent/blob/fd0d1bb164dd5dbd0f64c7a2d0e727cf7b13e777/control_v1.proto#L7-L11

michel-laterman and others added 2 commits May 21, 2024 10:32
Rename control proto package to cproto so it does not conflict with
elastic-agent-client import
@michel-laterman michel-laterman requested a review from a team as a code owner May 21, 2024 17:32
@michel-laterman michel-laterman requested review from belimawr and removed request for a team May 21, 2024 17:32
@michel-laterman michel-laterman changed the title Rename contol proto package to cproto Update elastic-agent-client and rename control proto package to cproto May 21, 2024
Copy link
Contributor

@belimawr belimawr left a comment

Choose a reason for hiding this comment

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

LGTM, but I have the same question as Craig: Does it break any compatibility with older/newer versions that will need to communicate with this agent?

@michel-laterman
Copy link
Contributor Author

I was able to upgrade successfully from 7.17.21-SNAPSHOT to 7.17.22-SNAPSHOT on a standalone install.
I was also able to upgrade afterwards to 8.15.0-SNAPSHOT on the same install.

@jlind23 jlind23 requested a review from ycombinator May 24, 2024 07:13
Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

Looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport bug Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants