Skip to content

Make it possible to set capabilities #366

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Make it possible to set capabilities #366

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
adcb1be
Make it possible to set capabilities
jaronoff97 Mar 24, 2025
cf4a028
formatting
jaronoff97 Mar 24, 2025
21667d9
update prototype
jaronoff97 Mar 27, 2025
3f61b9c
update from new spec changes
jaronoff97 Apr 16, 2025
4b81c39
Update to use setcapabilities and deprecate the old method
jaronoff97 May 26, 2025
a130307
update from feedback
jaronoff97 Jun 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion client/clientimpl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import (

const retryAfterHTTPHeader = "Retry-After"

var coreCapabilities = protobufs.AgentCapabilities_AgentCapabilities_ReportsStatus

func createAgentDescr() *protobufs.AgentDescription {
agentDescr := &protobufs.AgentDescription{
IdentifyingAttributes: []*protobufs.KeyValue{
Expand Down Expand Up @@ -129,6 +131,12 @@ func prepareClient(t *testing.T, settings *types.StartSettings, c OpAMPClient) {
prepareSettings(t, settings, c)
err := c.SetAgentDescription(createAgentDescr())
assert.NoError(t, err)
// We ignore the error here.
if settings.Capabilities != 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.

i had to ignore the error here because a later test depended on the checks happening in the start client

c.SetCapabilities(&settings.Capabilities)
} else {
c.SetCapabilities(&coreCapabilities)
}
}

func startClient(t *testing.T, settings types.StartSettings, client OpAMPClient) {
Expand Down Expand Up @@ -218,13 +226,26 @@ func TestStopCancellation(t *testing.T) {

func TestStartNoDescription(t *testing.T) {
testClients(t, func(t *testing.T, client OpAMPClient) {
setErr := client.SetCapabilities(&coreCapabilities)
assert.NoError(t, setErr)
settings := createNoServerSettings()
prepareSettings(t, &settings, client)
err := client.Start(context.Background(), settings)
assert.EqualValues(t, err, internal.ErrAgentDescriptionMissing)
})
}

func TestStartNoCapabilities(t *testing.T) {
testClients(t, func(t *testing.T, client OpAMPClient) {
setErr := client.SetAgentDescription(createAgentDescr())
require.NoError(t, setErr)
settings := createNoServerSettings()
prepareSettings(t, &settings, client)
err := client.Start(context.Background(), settings)
assert.EqualValues(t, internal.ErrCapabilitiesNotSet, err)
})
}

func TestSetInvalidAgentDescription(t *testing.T) {
testClients(t, func(t *testing.T, client OpAMPClient) {
settings := createNoServerSettings()
Expand Down Expand Up @@ -1622,7 +1643,6 @@ func TestMissingPackagesStateProvider(t *testing.T) {
protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses,
}
prepareClient(t, &settings, client)

assert.ErrorIs(t, client.Start(context.Background(), settings), internal.ErrPackagesStateProviderNotSet)

// Start a client.
Expand Down
20 changes: 13 additions & 7 deletions client/internal/clientcommon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
ErrReportsEffectiveConfigNotSet = errors.New("ReportsEffectiveConfig capability is not set")
ErrReportsRemoteConfigNotSet = errors.New("ReportsRemoteConfig capability is not set")
ErrPackagesStateProviderNotSet = errors.New("PackagesStateProvider must be set")
ErrCapabilitiesNotSet = errors.New("Capabilities is not set")
ErrAcceptsPackagesNotSet = errors.New("AcceptsPackages and ReportsPackageStatuses must be set")
ErrAvailableComponentsMissing = errors.New("AvailableComponents is nil")

Expand Down Expand Up @@ -97,13 +98,18 @@
if c.isStarted {
return errAlreadyStarted
}
capabilities := settings.Capabilities

// According to OpAMP spec this capability MUST be set, since all Agents MUST report status.
capabilities |= protobufs.AgentCapabilities_AgentCapabilities_ReportsStatus
if err := c.ClientSyncedState.SetCapabilities(&capabilities); err != nil {
return err
// Deprecated: Use client.SetCapabilities() instead.
if settings.Capabilities != 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.

@tigrannajaryan this technically could be a breaking change if someone isn't setting any capabilities, should I just default to setting reports status here and then in the future we remove this?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should emit a warning in the log if SetCapabilities is not called before Start. Later, we can make it an error. Then finally some time after that we remove Settings.Capabilities field.

In any of these 3 states, ReportsStatus should always be automatically added, regardless of how the Capabilities are set (via Settings or via method call).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tigrannajaryan i do make sure reportsstatus is always set (I do the add in this method as well in the clientcommon setcapabilities), but I'll add in a log right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tigrannajaryan should be all good now. PTAL 🙇

capabilities := settings.Capabilities
// According to OpAMP spec this capability MUST be set, since all Agents MUST report status.
capabilities |= protobufs.AgentCapabilities_AgentCapabilities_ReportsStatus
if err := c.ClientSyncedState.SetCapabilities(&capabilities); err != nil {
return err
}
}
if c.ClientSyncedState.Capabilities() == 0 {
return ErrCapabilitiesNotSet
}

Check warning on line 112 in client/internal/clientcommon.go

View check run for this annotation

Codecov / codecov/patch

client/internal/clientcommon.go#L112 

Added line #L112 was not covered by tests

if c.ClientSyncedState.AgentDescription() == nil {
return ErrAgentDescriptionMissing
Expand All @@ -111,7 +117,7 @@

// Prepare package statuses.
c.PackagesStateProvider = settings.PackagesStateProvider
if err := c.validateCapabilities(capabilities); err != nil {
if err := c.validateCapabilities(c.ClientSyncedState.Capabilities()); err != nil {
return err
}

Expand All @@ -129,8 +135,8 @@

var packageStatuses *protobufs.PackageStatuses
if c.PackagesStateProvider != nil &&
c.hasCapability(protobufs.AgentCapabilities_AgentCapabilities_AcceptsPackages) &&
c.hasCapability(protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses) {

Check warning on line 139 in client/internal/clientcommon.go

View check run for this annotation

Codecov / codecov/patch

client/internal/clientcommon.go#L138-L139 

Added lines #L138 - L139 were not covered by tests
// Set package status from the value previously saved in the PackagesStateProvider.
var err error
packageStatuses, err = settings.PackagesStateProvider.LastReportedStatuses()
Expand Down Expand Up @@ -519,8 +525,8 @@
if err := c.ClientSyncedState.SetCapabilities(capabilities); err != nil {
return err
}
// send the new customCapabilities to the Server
c.sender.NextMessage().Update(

Check warning on line 529 in client/internal/clientcommon.go

View check run for this annotation

Codecov / codecov/patch

client/internal/clientcommon.go#L528-L529 

Added lines #L528 - L529 were not covered by tests
func(msg *protobufs.AgentToServer) {
msg.Capabilities = uint64(c.ClientSyncedState.Capabilities())
},
Expand All @@ -528,8 +534,8 @@
c.sender.ScheduleSend()
return nil
}

// QUESTION: DO we want this?

Check warning on line 538 in client/internal/clientcommon.go

View check run for this annotation

Codecov / codecov/patch

client/internal/clientcommon.go#L537-L538 

Added lines #L537 - L538 were not covered by tests
// SetPackagesStateProvider allows the confgiguration of the packages state provider after start.
// func (c *ClientCommon) SetPackagesStateProvider(packagesStateProvider types.PackagesStateProvider) error {
// c.PackageSyncMutex.Lock()
Expand Down
3 changes: 1 addition & 2 deletions client/internal/clientstate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
errPackageStatusesMissing = errors.New("PackageStatuses is not set")
errServerProvidedAllPackagesHashNil = errors.New("ServerProvidedAllPackagesHash is nil")
errCustomCapabilitiesMissing = errors.New("CustomCapabilities is not set")
errCapabilitiesMissing = errors.New("Capabilities is not set")
errAvailableComponentsMissing = errors.New("AvailableComponents is not set")
)

Expand Down Expand Up @@ -217,8 +216,8 @@
// SetCapabilities sets the Capabilities in the state.
func (s *ClientSyncedState) SetCapabilities(capabilities *protobufs.AgentCapabilities) error {
if capabilities == nil {
return errCapabilitiesMissing
return ErrCapabilitiesNotSet
}

Check warning on line 220 in client/internal/clientstate.go

View check run for this annotation

Codecov / codecov/patch

client/internal/clientstate.go#L219-L220 

Added lines #L219 - L220 were not covered by tests

defer s.mutex.Unlock()
s.mutex.Lock()
Expand Down
1 change: 1 addition & 0 deletions client/types/startsettings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ type StartSettings struct {

// Defines the capabilities of the Agent. AgentCapabilities_ReportsStatus bit does not need to
// be set in this field, it will be set automatically since it is required by OpAMP protocol.
// Deprecated: Use client.SetCapabilities() instead.
Capabilities protobufs.AgentCapabilities

// EnableCompression can be set to true to enable the compression. Note that for WebSocket transport
Expand Down
Loading