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

[#417] Make pgagroal-admin provide JSON output #440

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

T1t4m1un
Copy link
Contributor

Since the output data is very simple, I directly use printf to output in JSON format in the original function.

I think my implementation is the simplest and most reliable implementation considering the complexity of the data. Of course, if you have a better idea, please tell me.

If there are other use cases that need support, please let me know ~ I will complete the documentation after completing all use cases~

@fluca1978
Copy link
Collaborator

@T1t4m1un thanks for the contribution, but I don't like the idea of using printf to print directly to the output.
I agree that the output format is quite simple, but the aim should be to have much more information in the JSON output, so the list array with the count and users subelements, the command name, version and all the other stuff we have for pgagroal-cli json output. In other words, please use the json.c stuff.

The aim is not to present output in brackets, it is to allow for automation, hence the output must be as complete as possible.

@T1t4m1un
Copy link
Contributor Author

@T1t4m1un thanks for the contribution, but I don't like the idea of using printf to print directly to the output. I agree that the output format is quite simple, but the aim should be to have much more information in the JSON output, so the list array with the count and users subelements, the command name, version and all the other stuff we have for pgagroal-cli json output. In other words, please use the json.c stuff.

OK, I will refactor it~

The aim is not to present output in brackets, it is to allow for automation, hence the output must be as complete as possible.

Could you give me some examples of the format? I don't clearly understand the format you envisioned😶‍🌫️

@fluca1978
Copy link
Collaborator

Could you give me some examples of the format? I don't clearly understand the format you envisioned😶‍🌫️

See https://github.com/agroal/pgagroal/blob/master/doc/CLI.md#json-output-format.
I think the command should give this kind of output:

$ pgagroal-admin user ls --format json

{
        "command":      {
                "name": "user ls",
                "status":       "OK",
                "error":        0,
                "exit-status":  0,
                "output":       {
                       "users":    {
                                        "count":        2,
                                        "list": [ "luca", "jesper" ]
                        }
                }
        },
        "application":  {
                "name": "pgagroal-admin",
                "major":        1,
                "minor":        7,
                "patch":        0,
                "version":      "1.7.0"
        }
}

or something simpler, but the above should be for free using the json.c stuff.

@T1t4m1un
Copy link
Contributor Author

image

@fluca1978 Hey Luca, I think this version is what you want~ However I'm not sure if these json manipulations should be encapsulated into a function, so I completed the functionality first. If you think encapsulation is necessary, please give me some suggestions on the name and where to place it~

I'll add this option into documentation if there's no other problems with this version~

Copy link
Collaborator

@fluca1978 fluca1978 left a comment

Choose a reason for hiding this comment

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

I think it is done, I would only change the assignment of subelements so to get them early in the json structure, as I commented.

@jesperpedersen PTAL

src/admin.c Outdated Show resolved Hide resolved
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.

None yet

2 participants