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.

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

Jump to bottom

Update settings api to use ResourceContext over ModificationInfo #1633

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

Update settings api to use ResourceContext over ModificationInfo #1633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2af6ac3
refactor: refactor client code to depend on `DownloadSettingObject` m…
tomazpu Oct 30, 2024
1f66165
feat: implement `DownloadSettingsObject` methods
tomazpu Oct 30, 2024
45340bd
chore: Add unit test to test modificationInfo returns same results as…
tomazpu Nov 12, 2024
7442a1e
chore: update branch after rebase
tomazpu Nov 21, 2024
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
64 changes: 64 additions & 0 deletions pkg/client/dtclient/settings_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"fmt"
"net/http"
"net/url"
"slices"
"strings"

"github.com/google/go-cmp/cmp"
Expand All @@ -41,6 +42,13 @@ import (
"github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/config/coordinate"
)

// SettingsResourceContext.Operations possibilities
const (
DeleteOperation = "delete"
WriteOperation = "write"
//ReadOperation = "read"
)

// DownloadSettingsObject is the response type for the ListSettings operation
type DownloadSettingsObject struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not move to a separate file

ExternalId string `json:"externalId"`
Expand All @@ -50,6 +58,55 @@ type DownloadSettingsObject struct {
Scope string `json:"scope"`
Value json.RawMessage `json:"value"`
ModificationInfo *SettingsModificationInfo `json:"modificationInfo"`
ResourceContext *SettingsResourceContext `json:"resourceContext"`
}

func (settingObject *DownloadSettingsObject) IsDeletable() bool {
if settingObject.ResourceContext != nil {
return slices.Contains(settingObject.ResourceContext.Operations, DeleteOperation)
}

if settingObject.ModificationInfo != nil {
return settingObject.ModificationInfo.Deletable
}

return true
}

func (settingObject *DownloadSettingsObject) IsModifiable() bool {
if settingObject.ResourceContext != nil {
return slices.Contains(settingObject.ResourceContext.Operations, WriteOperation)
}

if settingObject.ModificationInfo != nil {
return settingObject.ModificationInfo.Modifiable
}

return true
}

func (settingObject *DownloadSettingsObject) IsMovable() bool {
if settingObject.ResourceContext != nil {
//API allows the parameter to be optional, so more logic is needed to handle it
if settingObject.ResourceContext.Movable != nil {
return *settingObject.ResourceContext.Movable
}
return true
}

if settingObject.ModificationInfo != nil {
return settingObject.ModificationInfo.Movable
}

return true
}

func (settingObject *DownloadSettingsObject) GetModifiablePaths() []interface{} {
if settingObject.ResourceContext != nil {
return settingObject.ResourceContext.ModifiablePaths
}

return settingObject.ModificationInfo.ModifiablePaths
}

type SettingsModificationInfo struct {
Expand All @@ -60,6 +117,13 @@ type SettingsModificationInfo struct {
NonModifiablePaths []interface{} `json:"nonModifiablePaths"`
}

type SettingsResourceContext struct {
Operations []string `json:"operations"`
Movable *bool `json:"modifications:movable"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To the reviewer. I chose a pointer over its own struct type, the problem here is that the param Movable is optional, and logic code in method isMovable() depends on it.

If preferred, I could change it to a NullBool type and implement the unmarshaling myself. Since is not done anywhere else in the code i chose *bool over NullBool.

Copy link
Contributor

Choose a reason for hiding this comment

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

By now, I wasn't aware of the NullBool type in GO. But as I see it, it is part of the SQL library and for that reason wouldn't it even consider. For me pointer is just enough

ModifiablePaths []interface{} `json:"modifications:modifiablePaths"`
NonModifiablePaths []interface{} `json:"modifications:nonModifiablePaths"`
}

type UpsertSettingsOptions struct {
OverrideRetry *RetrySetting
InsertAfter string
Expand Down
53 changes: 53 additions & 0 deletions pkg/client/dtclient/settings_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1781,3 +1781,56 @@ func TestDeleteSettings(t *testing.T) {
})
}
}

func TestResourceContextWorksAsModificationInfo(t *testing.T) {
boolFalse := false
tests := []struct {
name string
modificationInfo SettingsModificationInfo
resourceContext SettingsResourceContext
}{
{
name: "only read item",
modificationInfo: SettingsModificationInfo{
Deletable: false,
Modifiable: false,
Movable: false,
ModifiablePaths: []interface{}{"apiColor", "thirdPartyApi"},
},
resourceContext: SettingsResourceContext{
Operations: []string{"read"},
Movable: &boolFalse,
ModifiablePaths: []interface{}{"apiColor", "thirdPartyApi"},
},
},
{
name: "deletable, modifiable, movable item - no movable param provided in resource context",
modificationInfo: SettingsModificationInfo{
Deletable: true,
Modifiable: true,
Movable: true,
ModifiablePaths: []interface{}{},
},
resourceContext: SettingsResourceContext{
Operations: []string{"read", "write", "delete"},
ModifiablePaths: []interface{}{},
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
settingObjectWithModificationInfo := DownloadSettingsObject{
ModificationInfo: &tt.modificationInfo,
}
settingObjectWithResourceContext := DownloadSettingsObject{
ResourceContext: &tt.resourceContext,
}
assert.Equal(t, settingObjectWithModificationInfo.IsDeletable(), settingObjectWithResourceContext.IsDeletable())
assert.Equal(t, settingObjectWithModificationInfo.IsMovable(), settingObjectWithResourceContext.IsMovable())
assert.Equal(t, settingObjectWithModificationInfo.IsModifiable(), settingObjectWithResourceContext.IsModifiable())

assert.Equal(t, settingObjectWithModificationInfo.GetModifiablePaths(), settingObjectWithResourceContext.GetModifiablePaths())
})
}
}
31 changes: 16 additions & 15 deletions pkg/delete/internal/setting/delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,14 @@ func Delete(ctx context.Context, c client.SettingsClient, entries []pointer.Dele
continue
}

objects, err := c.List(ctx, e.Type, dtclient.ListSettingsOptions{DiscardValue: true, Filter: filterFunc})
settingsObjects, err := c.List(ctx, e.Type, dtclient.ListSettingsOptions{DiscardValue: true, Filter: filterFunc})
if err != nil {
logger.Error("Could not fetch settings object: %v", err)
deleteErrs++
continue
}

if len(objects) == 0 {
if len(settingsObjects) == 0 {
if e.OriginObjectId != "" {
logger.Debug("No settings object found to delete. Could not find object with matching object id.")
continue
Expand All @@ -66,16 +66,16 @@ func Delete(ctx context.Context, c client.SettingsClient, entries []pointer.Dele
continue
}

for _, obj := range objects {
if obj.ModificationInfo != nil && !obj.ModificationInfo.Deletable {
logger.WithFields(field.F("object", obj)).Warn("Requested settings object with ID %s is not deletable.", obj.ObjectId)
for _, settingsObject := range settingsObjects {
if !settingsObject.IsDeletable() {
logger.WithFields(field.F("object", settingsObject)).Warn("Requested settings object with ID %s is not deletable.", settingsObject.ObjectId)
continue
}

logger.Debug("Deleting settings object with objectId %q.", obj.ObjectId)
err := c.Delete(ctx, obj.ObjectId)
logger.Debug("Deleting settings object with objectId %q.", settingsObject.ObjectId)
err := c.Delete(ctx, settingsObject.ObjectId)
if err != nil {
logger.Error("Failed to delete settings object with object ID %s: %v", obj.ObjectId, err)
logger.Error("Failed to delete settings object with object ID %s: %v", settingsObject.ObjectId, err)
deleteErrs++
}
}
Expand Down Expand Up @@ -129,23 +129,24 @@ func DeleteAll(ctx context.Context, c client.SettingsClient) error {
logger := logger.WithFields(field.Type(s))
logger.Info("Collecting objects of type %q...", s)

settings, err := c.List(ctx, s, dtclient.ListSettingsOptions{DiscardValue: true})
settingsObjects, err := c.List(ctx, s, dtclient.ListSettingsOptions{DiscardValue: true})
if err != nil {
logger.WithFields(field.Error(err)).Error("Failed to collect object for schema %q: %v", s, err)
errs++
continue
}

logger.Info("Deleting %d objects of type %q...", len(settings), s)
for _, setting := range settings {
if setting.ModificationInfo != nil && !setting.ModificationInfo.Deletable {
logger.Info("Deleting %d objects of type %q...", len(settingsObjects), s)
for _, settingsObject := range settingsObjects {
if !settingsObject.IsDeletable() {
//@Reviewer should here also be a log as it is in the Delete() method
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, it can be added. But if you doesn't see the purpose, I wouldn't

continue
}

logger.WithFields(field.F("object", setting)).Debug("Deleting settings object with objectId %q...", setting.ObjectId)
err := c.Delete(ctx, setting.ObjectId)
logger.WithFields(field.F("object", settingsObject)).Debug("Deleting settingsObjects object with objectId %q...", settingsObject.ObjectId)
err := c.Delete(ctx, settingsObject.ObjectId)
if err != nil {
logger.Error("Failed to delete settings object with object ID %s: %v", setting.ObjectId, err)
logger.Error("Failed to delete settingsObjects object with object ID %s: %v", settingsObject.ObjectId, err)
errs++
}
}
Expand Down
37 changes: 18 additions & 19 deletions pkg/download/settings/download.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,50 +182,49 @@ func asConcurrentErrMsg(err coreapi.APIError) string {
return fmt.Sprintf("%s\n%s", err.Error(), additionalMessage)
}

func convertAllObjects(objects []dtclient.DownloadSettingsObject, projectName string, ordered bool, filters Filters) []config.Config {
result := make([]config.Config, 0, len(objects))
func convertAllObjects(settingsObjects []dtclient.DownloadSettingsObject, projectName string, ordered bool, filters Filters) []config.Config {
result := make([]config.Config, 0, len(settingsObjects))
var previousConfig *config.Config = nil
for _, o := range objects {

if shouldFilterUnmodifiableSettings() && o.ModificationInfo != nil && !o.ModificationInfo.Modifiable && len(o.ModificationInfo.ModifiablePaths) == 0 {
log.WithFields(field.Type(o.SchemaId), field.F("object", o)).Debug("Discarded settings object %q (%s). Reason: Unmodifiable default setting.", o.ObjectId, o.SchemaId)
for _, settingsObject := range settingsObjects {
if shouldFilterUnmodifiableSettings() && !settingsObject.IsModifiable() && len(settingsObject.GetModifiablePaths()) == 0 {
log.WithFields(field.Type(settingsObject.SchemaId), field.F("object", settingsObject)).Debug("Discarded settings object %q (%s). Reason: Unmodifiable default setting.", settingsObject.ObjectId, settingsObject.SchemaId)
continue
}

// try to unmarshall settings value
var contentUnmarshalled map[string]interface{}
if err := json.Unmarshal(o.Value, &contentUnmarshalled); err != nil {
log.WithFields(field.Type(o.SchemaId), field.F("object", o)).Error("Unable to unmarshal JSON value of settings 2.0 object: %v", err)
if err := json.Unmarshal(settingsObject.Value, &contentUnmarshalled); err != nil {
log.WithFields(field.Type(settingsObject.SchemaId), field.F("object", settingsObject)).Error("Unable to unmarshal JSON value of settings 2.0 object: %v", err)
return result
}
// skip discarded settings objects
if shouldDiscard, reason := filters.Get(o.SchemaId).ShouldDiscard(contentUnmarshalled); shouldFilterSettings() && shouldDiscard {
log.WithFields(field.Type(o.SchemaId), field.F("object", o)).Debug("Discarded setting object %q (%s). Reason: %s", o.ObjectId, o.SchemaId, reason)
// skip discarded settings settingsObjects
Copy link
Contributor

Choose a reason for hiding this comment

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

to much of renaming :)

if shouldDiscard, reason := filters.Get(settingsObject.SchemaId).ShouldDiscard(contentUnmarshalled); shouldFilterSettings() && shouldDiscard {
log.WithFields(field.Type(settingsObject.SchemaId), field.F("object", settingsObject)).Debug("Discarded setting object %q (%s). Reason: %s", settingsObject.ObjectId, settingsObject.SchemaId, reason)
continue
}

indentedJson := jsonutils.MarshalIndent(o.Value)
indentedJson := jsonutils.MarshalIndent(settingsObject.Value)
// construct config object with generated config ID
configId := idutils.GenerateUUIDFromString(o.ObjectId)
configId := idutils.GenerateUUIDFromString(settingsObject.ObjectId)
c := config.Config{
Template: template.NewInMemoryTemplate(configId, string(indentedJson)),
Coordinate: coordinate.Coordinate{
Project: projectName,
Type: o.SchemaId,
Type: settingsObject.SchemaId,
ConfigId: configId,
},
Type: config.SettingsType{
SchemaId: o.SchemaId,
SchemaVersion: o.SchemaVersion,
SchemaId: settingsObject.SchemaId,
SchemaVersion: settingsObject.SchemaVersion,
},
Parameters: map[string]parameter.Parameter{
config.ScopeParameter: &value.ValueParameter{Value: o.Scope},
config.ScopeParameter: &value.ValueParameter{Value: settingsObject.Scope},
},
Skip: false,
OriginObjectId: o.ObjectId,
OriginObjectId: settingsObject.ObjectId,
}

if o.ModificationInfo != nil && o.ModificationInfo.Movable && ordered && (previousConfig != nil) {
if settingsObject.IsMovable() && ordered && (previousConfig != nil) {
c.Parameters[config.InsertAfterParameter] = reference.NewWithCoordinate(previousConfig.Coordinate, "id")
}
result = append(result, c)
Expand Down
Loading