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

fix: agentctl config dump not working with remote models #1863

Merged
merged 9 commits into from
Sep 19, 2022

Conversation

pemoticak
Copy link
Collaborator

@pemoticak pemoticak commented Aug 9, 2022

This PR:

  • fixes agentctl config dump to work with remote models (fix is very similar to fix: agentctl config history not working with remote models #1860)
  • refactors the scheduler client API to use a type more suited for JSON (un)marshalling
  • recorded proto messages are now (un)marshalled using protojson package instead of prototext
  • ModelList now registers the models that it fetches into the DefaultRegistry

These changes (mainly the ModelList one) fix some problems with trailing slashes in remote model key prefixes (for example when running agentctl model list/inspect).

Signed-off-by: Peter Motičák peter.moticak@pantheon.tech

Signed-off-by: Peter Motičák <peter.moticak@pantheon.tech>
Use the RecordedKVWithMetadata type that can be properly un/marshalled
from/to JSON.

Signed-off-by: Peter Motičák <peter.moticak@pantheon.tech>
Signed-off-by: Peter Motičák <peter.moticak@pantheon.tech>
Signed-off-by: Peter Motičák <peter.moticak@pantheon.tech>
* (Un)marshal RecordedProtoMessage as JSON instead of text

* Add ability to create custom dump formats using templates for remote models

* Fix the remaining agentctl e2e tests

Signed-off-by: Peter Motičák <peter.moticak@pantheon.tech>
Signed-off-by: Peter Motičák <peter.moticak@pantheon.tech>
@ondrej-fabry
Copy link
Member

is this ready for merge?

@pemoticak
Copy link
Collaborator Author

Yes the fix itself works and should be ready for merge but it doesn't have an e2e test. The test could be added in a separate PR at a later time though.

Signed-off-by: Peter Motičák <peter.moticak@pantheon.tech>
Signed-off-by: Peter Motičák <peter.moticak@pantheon.tech>
@ondrej-fabry ondrej-fabry merged commit e937109 into ligato:master Sep 19, 2022
@pemoticak pemoticak deleted the fix-dump branch September 30, 2022 09:27
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