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

Destination option for agama logs store #823

Merged
merged 18 commits into from Nov 15, 2023
Merged

Destination option for agama logs store #823

merged 18 commits into from Nov 15, 2023

Conversation

mchf
Copy link
Member

@mchf mchf commented Oct 27, 2023

Problem

When storing logs during installation it would be nice to be able to define destination path

Solution

added an option for agama logs store

Testing

  • Tested manually

Screenshots

TODO: add one

rust/agama-cli/src/logs.rs Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Oct 30, 2023

Coverage Status

coverage: 65.578% (-0.09%) from 65.669%
when pulling d7aa245 on logs_store_dest
into 9e62e88 on master.

@mchf mchf requested a review from imobachgs October 30, 2023 13:18
@mchf mchf marked this pull request as ready for review November 1, 2023 09:57
Copy link
Member

@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 would work a little on simplifying the parse_dest function. Beware, I have not tested my suggestions, so take them with a grain of salt 😉. Perhaps you should write a small unit test...

rust/agama-cli/src/logs.rs Outdated Show resolved Hide resolved
let err = io::Error::new(io::ErrorKind::InvalidInput, "Invalid destination path");

match dest {
None => Ok(default),
Copy link
Member

Choose a reason for hiding this comment

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

What if /tmp/agama_logs is already a directory? What if tomorrow we decide to change the default value? I would run the same checks whether it is the default or a value from the user.

This way, we could drop the match and save some nesting:

let mut buff = dest.unwrap_or(PathBuf::from(DEFAULT_RESULT));
let path = buff.as_path();

Copy link
Member

@imobachgs imobachgs Nov 2, 2023

Choose a reason for hiding this comment

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

BTW, why not buffer instead of buff? We are just saving two letters :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

if /tmp/agama_logs is already directory, then you will end up with /tmp/agama_logs.tar.bzip2 archive.

Copy link
Member Author

Choose a reason for hiding this comment

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

regarding buff`` well ... just matter of habit as i have been using buff```` and dest since I was at school, so it is kind of convention for me ;-) It is even used in man pages of such tools like ```cp```, ```rsync```, e.g. ```rsync``` defines its usage as ```rsync [OPTION...] SRC... [DEST]``` even in some options like ```--copy-dest=DIR```.

However, I don't have problem to extend it. It definitely is not stopper for me ;-)

Copy link
Member

Choose a reason for hiding this comment

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

Not even for me, I was curious especially about the "buff" case :-)

Copy link
Member

@imobachgs imobachgs Nov 15, 2023

Choose a reason for hiding this comment

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

About the /tmp/agama_logs, my point is that I would run the same checks in both cases. And in that case, you could save the match statement (too much nesting for my taste). Anyway, I am not going to block the approval. Please, fix the conflict in the changes file.

rust/agama-cli/src/logs.rs Outdated Show resolved Hide resolved
rust/agama-cli/src/logs.rs Outdated Show resolved Hide resolved
rust/agama-cli/src/logs.rs Outdated Show resolved Hide resolved
Copy link
Member

@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 clarified what I meant about the checks of /tmp/agama_logs.

let err = io::Error::new(io::ErrorKind::InvalidInput, "Invalid destination path");

match dest {
None => Ok(default),
Copy link
Member

@imobachgs imobachgs Nov 15, 2023

Choose a reason for hiding this comment

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

About the /tmp/agama_logs, my point is that I would run the same checks in both cases. And in that case, you could save the match statement (too much nesting for my taste). Anyway, I am not going to block the approval. Please, fix the conflict in the changes file.

@mchf
Copy link
Member Author

mchf commented Nov 15, 2023

I have clarified what I meant about the checks of /tmp/agama_logs.

I got it ... I'm just testing it. Just used an opportunity and cleaned even another part.

@mchf mchf merged commit 57a31cb into master Nov 15, 2023
1 check passed
@mchf mchf deleted the logs_store_dest branch November 15, 2023 12:45
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

5 participants