Skip to content

Commit

Permalink
Consistently declare Protobuf enums (#115)
Browse files Browse the repository at this point in the history 
Enums were not consistently declared. Some were at the global scope,
some others wire nested in messages. This moves all enums to the global
scope and uses most of the official style guide rules here:
https://developers.google.com/protocol-buffers/docs/style#enums

Unfortunately this results in long and stuttering names of enum values in
the code but this change is necessary to future-proof the proto and ensure
no enum names conflict when we add new enums and values (all enum value
names are in one namespace).

I don't like the end result but not doing it appears to be worse.
We have seen this first-hand in OTLP proto where enum names are
inconsistent and are now impossible to fix due to backward compatibility
concerns.

Note: I ignored the rule to CAPITALIZE names and use underscores
because if I do that the names in the code become unreadable walls
of text.
  • Loading branch information
@tigrannajaryan
tigrannajaryan authored Sep 20, 2022
1 parent 89eeb30 commit dc8c620
Show file tree
Hide file tree
Showing 14 changed files with 963 additions and 902 deletions.
62 changes: 33 additions & 29 deletions client/clientimpl_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ func TestFirstStatusReport(t *testing.T) {
atomic.AddInt64(&remoteConfigReceived, 1)
},
},
Capabilities: protobufs.AgentCapabilities_AcceptsRemoteConfig,
Capabilities: protobufs.AgentCapabilities_AgentCapabilities_AcceptsRemoteConfig,
}
settings.OpAMPServerURL = "ws://" + srv.Endpoint
startClient(t, settings, client)
Expand Down Expand Up @@ -442,7 +442,7 @@ func TestSetEffectiveConfig(t *testing.T) {
return sendConfig, nil
},
},
Capabilities: protobufs.AgentCapabilities_ReportsEffectiveConfig,
Capabilities: protobufs.AgentCapabilities_AgentCapabilities_ReportsEffectiveConfig,
}
settings.OpAMPServerURL = "ws://" + srv.Endpoint
prepareClient(t, &settings, client)
Expand Down Expand Up @@ -668,11 +668,11 @@ func TestConnectionSettings(t *testing.T) {
return nil
},
},
Capabilities: protobufs.AgentCapabilities_ReportsOwnTraces |
protobufs.AgentCapabilities_ReportsOwnMetrics |
protobufs.AgentCapabilities_ReportsOwnLogs |
protobufs.AgentCapabilities_AcceptsOtherConnectionSettings |
protobufs.AgentCapabilities_AcceptsOpAMPConnectionSettings,
Capabilities: protobufs.AgentCapabilities_AgentCapabilities_ReportsOwnTraces |
protobufs.AgentCapabilities_AgentCapabilities_ReportsOwnMetrics |
protobufs.AgentCapabilities_AgentCapabilities_ReportsOwnLogs |
protobufs.AgentCapabilities_AgentCapabilities_AcceptsOtherConnectionSettings |
protobufs.AgentCapabilities_AgentCapabilities_AcceptsOpAMPConnectionSettings,
}
settings.OpAMPServerURL = "ws://" + srv.Endpoint
prepareClient(t, &settings, client)
Expand Down Expand Up @@ -703,7 +703,7 @@ func TestReportAgentDescription(t *testing.T) {
// Start a client.
settings := types.StartSettings{
OpAMPServerURL: "ws://" + srv.Endpoint,
Capabilities: protobufs.AgentCapabilities_ReportsEffectiveConfig,
Capabilities: protobufs.AgentCapabilities_AgentCapabilities_ReportsEffectiveConfig,
}
prepareClient(t, &settings, client)

Expand Down Expand Up @@ -732,7 +732,7 @@ func TestReportAgentDescription(t *testing.T) {
// Ask client for full AgentDescription.
return &protobufs.ServerToAgent{
InstanceUid: msg.InstanceUid,
Flags: protobufs.ServerToAgent_ReportFullState,
Flags: protobufs.ServerToAgentFlags_ServerToAgentFlags_ReportFullState,
}
})

Expand Down Expand Up @@ -766,7 +766,8 @@ func TestReportAgentHealth(t *testing.T) {
// Start a client.
settings := types.StartSettings{
OpAMPServerURL: "ws://" + srv.Endpoint,
Capabilities: protobufs.AgentCapabilities_ReportsEffectiveConfig | protobufs.AgentCapabilities_ReportsHealth,
Capabilities: protobufs.AgentCapabilities_AgentCapabilities_ReportsEffectiveConfig |
protobufs.AgentCapabilities_AgentCapabilities_ReportsHealth,
}
prepareClient(t, &settings, client)

Expand Down Expand Up @@ -804,7 +805,7 @@ func TestReportAgentHealth(t *testing.T) {
// Ask client for full AgentDescription.
return &protobufs.ServerToAgent{
InstanceUid: msg.InstanceUid,
Flags: protobufs.ServerToAgent_ReportFullState,
Flags: protobufs.ServerToAgentFlags_ServerToAgentFlags_ReportFullState,
}
})

Expand Down Expand Up @@ -873,7 +874,7 @@ func TestReportEffectiveConfig(t *testing.T) {
// Ask client for full AgentDescription.
return &protobufs.ServerToAgent{
InstanceUid: msg.InstanceUid,
Flags: protobufs.ServerToAgent_ReportFullState,
Flags: protobufs.ServerToAgentFlags_ServerToAgentFlags_ReportFullState,
}
})

Expand Down Expand Up @@ -913,20 +914,21 @@ func verifyRemoteConfigUpdate(t *testing.T, successCase bool, expectStatus *prot
client.SetRemoteConfigStatus(
&protobufs.RemoteConfigStatus{
LastRemoteConfigHash: msg.RemoteConfig.ConfigHash,
Status: protobufs.RemoteConfigStatus_APPLIED,
Status: protobufs.RemoteConfigStatuses_RemoteConfigStatuses_APPLIED,
})
} else {
client.SetRemoteConfigStatus(
&protobufs.RemoteConfigStatus{
LastRemoteConfigHash: msg.RemoteConfig.ConfigHash,
Status: protobufs.RemoteConfigStatus_FAILED,
Status: protobufs.RemoteConfigStatuses_RemoteConfigStatuses_FAILED,
ErrorMessage: "cannot update remote config",
})
}
}
},
},
Capabilities: protobufs.AgentCapabilities_AcceptsRemoteConfig | protobufs.AgentCapabilities_ReportsRemoteConfig,
Capabilities: protobufs.AgentCapabilities_AgentCapabilities_AcceptsRemoteConfig |
protobufs.AgentCapabilities_AgentCapabilities_ReportsRemoteConfig,
}
prepareClient(t, &settings, client)

Expand Down Expand Up @@ -977,7 +979,7 @@ func verifyRemoteConfigUpdate(t *testing.T, successCase bool, expectStatus *prot
return &protobufs.ServerToAgent{
InstanceUid: msg.InstanceUid,
// Ask client to report full status.
Flags: protobufs.ServerToAgent_ReportFullState,
Flags: protobufs.ServerToAgentFlags_ServerToAgentFlags_ReportFullState,
}
})

Expand Down Expand Up @@ -1011,15 +1013,15 @@ func TestRemoteConfigUpdate(t *testing.T) {
name: "success",
success: true,
expectedStatus: &protobufs.RemoteConfigStatus{
Status: protobufs.RemoteConfigStatus_APPLIED,
Status: protobufs.RemoteConfigStatuses_RemoteConfigStatuses_APPLIED,
ErrorMessage: "",
},
},
{
name: "fail",
success: false,
expectedStatus: &protobufs.RemoteConfigStatus{
Status: protobufs.RemoteConfigStatus_FAILED,
Status: protobufs.RemoteConfigStatuses_RemoteConfigStatuses_FAILED,
ErrorMessage: "cannot update remote config",
},
},
Expand Down Expand Up @@ -1077,7 +1079,8 @@ func verifyUpdatePackages(t *testing.T, testCase packageTestCase) {
OnMessageFunc: onMessageFunc,
},
PackagesStateProvider: localPackageState,
Capabilities: protobufs.AgentCapabilities_AcceptsPackages | protobufs.AgentCapabilities_ReportsPackageStatuses,
Capabilities: protobufs.AgentCapabilities_AgentCapabilities_AcceptsPackages |
protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses,
}
prepareClient(t, &settings, client)

Expand Down Expand Up @@ -1120,10 +1123,10 @@ func verifyUpdatePackages(t *testing.T, testCase packageTestCase) {
continue
}
switch pkgStatus.Status {
case protobufs.PackageStatus_InstallFailed:
case protobufs.PackageStatusEnum_PackageStatusEnum_InstallFailed:
assert.Contains(t, pkgStatus.ErrorMessage, pkgExpected.ErrorMessage)

case protobufs.PackageStatus_Installed:
case protobufs.PackageStatusEnum_PackageStatusEnum_Installed:
assert.EqualValues(t, pkgExpected.AgentHasHash, pkgStatus.AgentHasHash)
assert.EqualValues(t, pkgExpected.AgentHasVersion, pkgStatus.AgentHasVersion)
assert.Empty(t, pkgStatus.ErrorMessage)
Expand Down Expand Up @@ -1166,7 +1169,7 @@ func verifyUpdatePackages(t *testing.T, testCase packageTestCase) {

if compressedReceived {
// Ask for full report again.
response.Flags = protobufs.ServerToAgent_ReportFullState
response.Flags = protobufs.ServerToAgentFlags_ServerToAgentFlags_ReportFullState
} else {
// Keep triggering status report by setting AgentDescription
// until the compressed PackageStatuses arrives.
Expand Down Expand Up @@ -1218,7 +1221,7 @@ func createPackageTestCase(name string, downloadSrv *httptest.Server) packageTes
available: &protobufs.PackagesAvailable{
Packages: map[string]*protobufs.PackageAvailable{
"package1": {
Type: protobufs.PackageAvailable_TopLevelPackage,
Type: protobufs.PackageType_PackageType_TopLevel,
Version: "1.0.0",
File: &protobufs.DownloadableFile{
DownloadUrl: downloadSrv.URL + packageFileURL,
Expand All @@ -1238,7 +1241,7 @@ func createPackageTestCase(name string, downloadSrv *httptest.Server) packageTes
AgentHasHash: []byte{1, 2, 3},
ServerOfferedVersion: "1.0.0",
ServerOfferedHash: []byte{1, 2, 3},
Status: protobufs.PackageStatus_Installed,
Status: protobufs.PackageStatusEnum_PackageStatusEnum_Installed,
ErrorMessage: "",
},
},
Expand All @@ -1263,7 +1266,7 @@ func TestUpdatePackages(t *testing.T) {
// A case when downloading the file fails because the URL is incorrect.
notFound := createPackageTestCase("downloadable file not found", downloadSrv)
notFound.available.Packages["package1"].File.DownloadUrl = downloadSrv.URL + "/notfound"
notFound.expectedStatus.Packages["package1"].Status = protobufs.PackageStatus_InstallFailed
notFound.expectedStatus.Packages["package1"].Status = protobufs.PackageStatusEnum_PackageStatusEnum_InstallFailed
notFound.expectedStatus.Packages["package1"].ErrorMessage = "cannot download"
tests = append(tests, notFound)

Expand Down Expand Up @@ -1359,8 +1362,9 @@ func TestMissingPackagesStateProvider(t *testing.T) {
testClients(t, func(t *testing.T, client OpAMPClient) {
// Start a client.
settings := types.StartSettings{
Callbacks: types.CallbacksStruct{},
Capabilities: protobufs.AgentCapabilities_AcceptsPackages | protobufs.AgentCapabilities_ReportsPackageStatuses,
Callbacks: types.CallbacksStruct{},
Capabilities: protobufs.AgentCapabilities_AgentCapabilities_AcceptsPackages |
protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses,
}
prepareClient(t, &settings, client)

Expand All @@ -1371,7 +1375,7 @@ func TestMissingPackagesStateProvider(t *testing.T) {
settings = types.StartSettings{
Callbacks: types.CallbacksStruct{},
PackagesStateProvider: localPackageState,
Capabilities: protobufs.AgentCapabilities_AcceptsPackages,
Capabilities: protobufs.AgentCapabilities_AgentCapabilities_AcceptsPackages,
}
prepareClient(t, &settings, client)

Expand All @@ -1381,7 +1385,7 @@ func TestMissingPackagesStateProvider(t *testing.T) {
settings = types.StartSettings{
Callbacks: types.CallbacksStruct{},
PackagesStateProvider: localPackageState,
Capabilities: protobufs.AgentCapabilities_ReportsPackageStatuses,
Capabilities: protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses,
}
prepareClient(t, &settings, client)

Expand Down
20 changes: 10 additions & 10 deletions client/internal/clientcommon.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,21 +76,21 @@ func (c *ClientCommon) PrepareStart(
c.Capabilities = settings.Capabilities

// According to OpAMP spec this capability MUST be set, since all Agents MUST report status.
c.Capabilities |= protobufs.AgentCapabilities_ReportsStatus
c.Capabilities |= protobufs.AgentCapabilities_AgentCapabilities_ReportsStatus

if c.ClientSyncedState.AgentDescription() == nil {
return ErrAgentDescriptionMissing
}

if c.Capabilities&protobufs.AgentCapabilities_ReportsHealth != 0 && c.ClientSyncedState.Health() == nil {
if c.Capabilities&protobufs.AgentCapabilities_AgentCapabilities_ReportsHealth != 0 && c.ClientSyncedState.Health() == nil {
return ErrAgentHealthMissing
}

// Prepare remote config status.
if settings.RemoteConfigStatus == nil {
// RemoteConfigStatus is not provided. Start with empty.
settings.RemoteConfigStatus = &protobufs.RemoteConfigStatus{
Status: protobufs.RemoteConfigStatus_UNSET,
Status: protobufs.RemoteConfigStatuses_RemoteConfigStatuses_UNSET,
}
}

Expand All @@ -102,8 +102,8 @@ func (c *ClientCommon) PrepareStart(
c.PackagesStateProvider = settings.PackagesStateProvider
var packageStatuses *protobufs.PackageStatuses
if settings.PackagesStateProvider != nil {
if c.Capabilities&protobufs.AgentCapabilities_AcceptsPackages == 0 ||
c.Capabilities&protobufs.AgentCapabilities_ReportsPackageStatuses == 0 {
if c.Capabilities&protobufs.AgentCapabilities_AgentCapabilities_AcceptsPackages == 0 ||
c.Capabilities&protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses == 0 {
return ErrAcceptsPackagesNotSet
}

Expand All @@ -114,8 +114,8 @@ func (c *ClientCommon) PrepareStart(
return err
}
} else {
if c.Capabilities&protobufs.AgentCapabilities_AcceptsPackages != 0 ||
c.Capabilities&protobufs.AgentCapabilities_ReportsPackageStatuses != 0 {
if c.Capabilities&protobufs.AgentCapabilities_AgentCapabilities_AcceptsPackages != 0 ||
c.Capabilities&protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses != 0 {
return ErrPackagesStateProviderNotSet
}
}
Expand Down Expand Up @@ -263,7 +263,7 @@ func (c *ClientCommon) SetHealth(health *protobufs.AgentHealth) error {
// UpdateEffectiveConfig fetches the current local effective config using
// GetEffectiveConfig callback and sends it to the Server using provided Sender.
func (c *ClientCommon) UpdateEffectiveConfig(ctx context.Context) error {
if c.Capabilities&protobufs.AgentCapabilities_ReportsEffectiveConfig == 0 {
if c.Capabilities&protobufs.AgentCapabilities_AgentCapabilities_ReportsEffectiveConfig == 0 {
return ErrReportsEffectiveConfigNotSet
}

Expand Down Expand Up @@ -294,7 +294,7 @@ func (c *ClientCommon) UpdateEffectiveConfig(ctx context.Context) error {
// It also remembers the new RemoteConfigStatus in the client state so that it can be
// sent to the Server when the Server asks for it.
func (c *ClientCommon) SetRemoteConfigStatus(status *protobufs.RemoteConfigStatus) error {
if c.Capabilities&protobufs.AgentCapabilities_ReportsRemoteConfig == 0 {
if c.Capabilities&protobufs.AgentCapabilities_AgentCapabilities_ReportsRemoteConfig == 0 {
return ErrReportsRemoteConfigNotSet
}

Expand Down Expand Up @@ -329,7 +329,7 @@ func (c *ClientCommon) SetRemoteConfigStatus(status *protobufs.RemoteConfigStatu
// It also remembers the new PackageStatuses in the client state so that it can be
// sent to the Server when the Server asks for it.
func (c *ClientCommon) SetPackageStatuses(statuses *protobufs.PackageStatuses) error {
if c.Capabilities&protobufs.AgentCapabilities_ReportsPackageStatuses == 0 {
if c.Capabilities&protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses == 0 {
return errReportsPackageStatusesNotSet
}

Expand Down
2 changes: 1 addition & 1 deletion client/internal/inmempackagestore.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func (l *InMemPackagesStore) PackageState(packageName string) (state types.Packa
return types.PackageState{Exists: false}, nil
}

func (l *InMemPackagesStore) CreatePackage(packageName string, typ protobufs.PackageAvailable_PackageType) error {
func (l *InMemPackagesStore) CreatePackage(packageName string, typ protobufs.PackageType) error {
l.pkgState[packageName] = types.PackageState{
Exists: true,
Type: typ,
Expand Down
10 changes: 5 additions & 5 deletions client/internal/packagessyncer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ func (s *packagesSyncer) syncPackage(
// Package is of wrong type. Need to re-create it. So, delete it.
if err := s.localState.DeletePackage(pkgName); err != nil {
err = fmt.Errorf("cannot delete existing version of package %s: %v", pkgName, err)
status.Status = protobufs.PackageStatus_InstallFailed
status.Status = protobufs.PackageStatusEnum_PackageStatusEnum_InstallFailed
status.ErrorMessage = err.Error()
return err
}
Expand All @@ -182,15 +182,15 @@ func (s *packagesSyncer) syncPackage(
}

// Report that we are beginning to install it.
status.Status = protobufs.PackageStatus_Installing
status.Status = protobufs.PackageStatusEnum_PackageStatusEnum_Installing
_ = s.reportStatuses(true)

if mustCreate {
// Make sure the package exists.
err = s.localState.CreatePackage(pkgName, pkgAvail.Type)
if err != nil {
err = fmt.Errorf("cannot create package %s: %v", pkgName, err)
status.Status = protobufs.PackageStatus_InstallFailed
status.Status = protobufs.PackageStatusEnum_PackageStatusEnum_InstallFailed
status.ErrorMessage = err.Error()
return err
}
Expand All @@ -203,14 +203,14 @@ func (s *packagesSyncer) syncPackage(
pkgLocal.Hash = pkgAvail.Hash
pkgLocal.Version = pkgAvail.Version
if err := s.localState.SetPackageState(pkgName, pkgLocal); err == nil {
status.Status = protobufs.PackageStatus_Installed
status.Status = protobufs.PackageStatusEnum_PackageStatusEnum_Installed
status.AgentHasHash = pkgAvail.Hash
status.AgentHasVersion = pkgAvail.Version
}
}

if err != nil {
status.Status = protobufs.PackageStatus_InstallFailed
status.Status = protobufs.PackageStatusEnum_PackageStatusEnum_InstallFailed
status.ErrorMessage = err.Error()
}
_ = s.reportStatuses(true)
Expand Down
Loading

0 comments on commit dc8c620

Please sign in to comment.