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

chore: improve organization of ./devimint #3798

Merged
merged 1 commit into from Dec 3, 2023

Conversation

mayrf
Copy link
Contributor

@mayrf mayrf commented Nov 30, 2023

Takes care of #3667. Title and description seem a bit contradictory, I hope interpreted it correctly.

@mayrf mayrf requested review from a team as code owners November 30, 2023 17:05
Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (361a16f) 57.55% compared to head (76cad30) 57.55%.
Report is 13 commits behind head on master.

Files Patch % Lines
devimint/src/main.rs 0.00% 13 Missing ⚠️
devimint/src/federation.rs 0.00% 4 Missing ⚠️
devimint/src/bin/faucet.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3798   +/-   ##
=======================================
  Coverage   57.55%   57.55%           
=======================================
  Files         193      193           
  Lines       42173    42189   +16     
=======================================
+ Hits        24272    24282   +10     
- Misses      17901    17907    +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mayrf mayrf force-pushed the rename-FM_DATA_DIR branch 2 times, most recently from d68d3e9 to 0a516a8 Compare November 30, 2023 17:38
@mayrf mayrf marked this pull request as draft November 30, 2023 18:40
@dpc
Copy link
Contributor

dpc commented Nov 30, 2023

While it's a step in the right direction if you look at data_dir it actually points at /tmp/.../cfg and contains client config/data. Not only I'd like to rename server-X to fedimint-X but also move it on level up or something similiar, because fedimintd data directory is not client config/data, and other services already have separate directories.

@mayrf Thanks for looking into it BTW.

@mayrf
Copy link
Contributor Author

mayrf commented Dec 1, 2023

Yeah, and I also ran into problem with the tests. I am still not sure I get what you have in mind exactly.
Do I understand correctly that you would imagine a structure like this?

/tmp/devimint-.../{new_named_dir}/config
/tmp/devimint-.../{new_named_dir}/fedimint-X

If yes, do you have a naming-suggestion for the new dir?

@dpc
Copy link
Contributor

dpc commented Dec 1, 2023

I don't want {new_named_dir} part. But I do want data_dir and similiar in the code and env variables to be renamed to client_dir or something like that and:

/tmp/devimint-.../client # instead of current `/tmp/devimint-.../cfg`
/tmp/devimint-.../fedimint-x # instead of current `/tmp/devimint-.../cfg/server-0

@mayrf

@mayrf mayrf marked this pull request as ready for review December 2, 2023 12:17
Copy link
Contributor

@elsirion elsirion left a comment

Choose a reason for hiding this comment

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

Concept ACK, I'll leave it to @dpc to verify this is what he wanted.

Copy link
Contributor

@dpc dpc left a comment

Choose a reason for hiding this comment

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

Best devimint state dir, eva.

@dpc dpc added this pull request to the merge queue Dec 3, 2023
Merged via the queue into fedimint:master with commit 98c6e27 Dec 3, 2023
20 checks passed
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

3 participants