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

add subscription data source #38

Merged
merged 1 commit into from
Jan 12, 2024
Merged

add subscription data source #38

merged 1 commit into from
Jan 12, 2024

Conversation

rnorton5432
Copy link
Contributor

Felipe has run into an issue with the non-root-user skill, since there is no longer an appropriate method for querying fields off the SyncRequest metadata. This PR adds the SubscriptionDataSource to resolve that blindspot.

SubscriptionDataSource allows querying of objects directly from the subscription results array. It is beneficial to use this data source when it is useful to query by name, possibly allowing earlier data sources to pick up the query first (e.g. local mocking).

I've also updated the local fixed data source to be a prepend action instead of append, to ensure that any local mocks are always handled first. With this, the order of options registered should not impact query behaviour.

SubscriptionDataSource allows querying of objects directly from the subscription results array. It is beneficial to use this data source when it is useful to query by name, possibly allowing earlier data sources to pick up the query first (e.g. local mocking).

I've also updated the local fixed data source to be a prepend action instead of append, to ensure that any local mocks are always handled first. With this, the order of options registered should not impact query behaviour.
@chrispatrick
Copy link
Collaborator

Hmm, while I think this will probably work, I'm a little wary about how complex all the data source logic is getting...

@rnorton5432
Copy link
Contributor Author

I'm a little wary about how complex all the data source logic is getting...

I'd love to revisit all this when we're ready to make some broader changes to local evaluation as a whole. For now, at least, I think each data source is small enough and simple enough to be easily maintainable.

@rnorton5432 rnorton5432 merged commit 4627967 into main Jan 12, 2024
2 checks passed
return nil, nil
}

if ix >= len(ds.subscriptionResults) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is right. subscriptionResults is an array of arrays. The first array is a set of results and the second array represents the different fields within each result.

}

func (ds SubscriptionDataSource) Query(_ context.Context, queryName string, _ string, _ map[string]interface{}, output interface{}) (*QueryResponse, error) {
ix, ok := ds.queryIndexes[queryName]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this is just personal preference, but I find it much harder to read when every entity is abbreviated.

return nil, fmt.Errorf("can't get subscription query %s (index %d) from result length %d", queryName, ix, len(ds.subscriptionResults))
}

err := edn.Unmarshal(ds.subscriptionResults[0][ix], output)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is also going to have issues anyway if we need to mock fields that appear in different orders in different subscriptions. Maybe we should look at changing go-skill to support supplying keys in the subscription so that it returns a map for each result instead of an array.

return nil, fmt.Errorf("can't get subscription query %s (index %d) from result length %d", queryName, ix, len(ds.subscriptionResults))
}

err := edn.Unmarshal(ds.subscriptionResults[0][ix], output)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are also cases where subscriptions can correctly return more than one result (see quality gate policy), so I'm not sure we should hardcode this to 0

@cdupuis cdupuis deleted the subscription-data-source branch February 2, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants