Skip to content

Refactoring: Create an http client to handle profile actions#2778

Merged
mchf merged 13 commits intomasterfrom
profile_client
Nov 11, 2025
Merged

Refactoring: Create an http client to handle profile actions#2778
mchf merged 13 commits intomasterfrom
profile_client

Conversation

@mchf
Copy link
Copy Markdown
Contributor

@mchf mchf commented Oct 3, 2025

Problem

Not a real problem, just something what was overlooked when during implementation.

Solution

Create an http client to handle profile validation and other actions to use similar approach as in other cases.

Copy link
Copy Markdown
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

I have revisited the approach and I have a few comments.

First of all, I am not a big fan of our current API, where we a) include the "path" or the "url" in the query string or b) we include the content of the file in the body. Why not use the body of the request for that? I know it was not introduced on this PR, but we might take the chance to make it better.

We are still constructing the profile client in separate functions (at least in three different places). I do not see the point (and the name of the functions are misleading to me).

Now, some behavior I did not expect (all of them happen in master too):

  1. Evaluating a profile using a URL that does not exist does not complain.
  2. Evaluating a local file using a remote Agama host (--host) does not seem to work.
  3. I cannot get the content of the file using the stdin, although the help says it is supported.
  4. Using --local does not work for me (e.g., agama --local config validate tw.json). It says I am not authenticated.

However, take them with a grain of salt, because it could be some problem in the environment. I need to have a closer look.

@mchf
Copy link
Copy Markdown
Contributor Author

mchf commented Oct 7, 2025

  1. Using --local does not work for me (e.g., agama --local config validate tw.json). It says I am not authenticated.

--local is config validate specific option as was decided here #2670. So, proper usage is
agama config validate --local /tmp/agama-valid.json

Then you get:
agama-local

Note that screenshot contains debugging output Validate subcommand local: true which is not present in production. The screenshot contains output of agama-cli built from this PR.

Copy link
Copy Markdown
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

Hi!

  1. The *_client functions are still there. If you want to keep them, it is fine. But, please, find a better name.
  2. Not related to this PR, I do not think CliInput should be responsible for converting to a query parameter, a body for a JSON call, etc. But I can live with that until the following refactoring :)
  3. The POST actions should use JSON in their bodies instead of a a=b format.

I do not want to make this longer, but it would be nice to at least address points 1 and 3. WDYT?

@mchf
Copy link
Copy Markdown
Contributor Author

mchf commented Nov 4, 2025

Hi!

  1. The *_client functions are still there. If you want to keep them, it is fine. But, please, find a better name.

... this is "in between step" and in the end I plan to rename them. The main focus currently was on removing those queries. However, I plan to keep them (with better names) as they are called on several places and it seems cleaner to me to call one function instead of that long client creation.

  1. Not related to this PR, I do not think CliInput should be responsible for converting to a query parameter, a body for a JSON call, etc. But I can live with that until the following refactoring :)

... I think that I'll be able to remove most of those "query" functions in the final cleanup ... at least I hope so.

  1. The POST actions should use JSON in their bodies instead of a a=b format.

... in fact it is posted as json (client.post encapsulates the content as a json). I'll polish it a bit.

I do not want to make this longer, but it would be nice to at least address points 1 and 3. WDYT?

... definitely that's why I asked before stating it as a final version ;-)

@mchf
Copy link
Copy Markdown
Contributor Author

mchf commented Nov 10, 2025

Sorry I did a force push because I accidentally put an unwanted modification of Gemfile.lock into one of previous commits

@mchf mchf requested a review from imobachgs November 10, 2025 09:41
Copy link
Copy Markdown
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

It looks better now. Please, add the changes entry and so on.

@mchf mchf requested a review from imobachgs November 10, 2025 14:48
@coveralls
Copy link
Copy Markdown

coveralls commented Nov 10, 2025

Coverage Status

coverage: 64.149% (+0.6%) from 63.569%
when pulling b839cb2 on profile_client
into 62f6f2b on master.

Copy link
Copy Markdown
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

Please, add the reference and you are clear to merge. Thank! :-)

Mon Nov 10 14:35:25 UTC 2025 - Michal Filka <mfilka@suse.com>

- Created ProfileClient based on HTTP API base client for profile
validation requests and refactored / cleaned relevant code
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please, add the reference (gh#agama-project/agaam#2778).

@mchf mchf merged commit 52366f5 into master Nov 11, 2025
13 checks passed
@mchf mchf deleted the profile_client branch November 11, 2025 09:08
imobachgs added a commit that referenced this pull request Nov 12, 2025
## Problem

By mistake previous refactoring (gh##2778) removed
printing of resulting profile

Now output looks like:
<img width="967" height="505" alt="agama_generate_config"
src="https://github.com/user-attachments/assets/b4444309-a9c0-46ef-8541-386190bd9336"
/>
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