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

feat(core): make a repository dir for each network #239

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

vincenzopalazzo
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo commented Feb 7, 2024

This is the more sane thing to do because each network can have a different repository version.

Fixes: #234
Fixes #233

Copy link

netlify bot commented Feb 7, 2024

Deploy Preview for coffee-docs canceled.

Name Link
🔨 Latest commit 8615159
🔍 Latest deploy log https://app.netlify.com/sites/coffee-docs/deploys/66447145c3ccc500087ded41

Copy link
Collaborator

@tareknaser tareknaser left a comment

Choose a reason for hiding this comment

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

I also have a concern. The code in this PR handles moving the global repository to be network specific but does it handle the information in coffee configuration as well?

So, if we run coffee nurse and then see coffee remote list for a network. is the output sane?

coffee_core/src/nurse/strategy.rs Outdated Show resolved Hide resolved
coffee_core/src/nurse/strategy.rs Outdated Show resolved Hide resolved
git-bugreport-2024-02-07-1353.txt Outdated Show resolved Hide resolved
coffee_lib/src/types/mod.rs Outdated Show resolved Hide resolved
coffee_core/src/coffee.rs Outdated Show resolved Hide resolved
coffee_core/src/coffee.rs Outdated Show resolved Hide resolved
coffee_core/src/nurse/strategy.rs Outdated Show resolved Hide resolved
coffee_cmd/src/coffee_term/command_show.rs Outdated Show resolved Hide resolved
coffee_core/src/coffee.rs Outdated Show resolved Hide resolved
@vincenzopalazzo vincenzopalazzo force-pushed the macros/moving-repository-dir branch from f145b1f to 1272fa5 Compare February 13, 2024 23:23
@vincenzopalazzo vincenzopalazzo marked this pull request as ready for review February 13, 2024 23:23
@vincenzopalazzo
Copy link
Contributor Author

ok I pushed the review you suggested @tareknaser tomorrow I will take a look if this PR is completed, but I guess yes?

I m not sure just when run the nurse command

@tareknaser
Copy link
Collaborator

just a reminder that #239 (review) is still not resolved

@vincenzopalazzo vincenzopalazzo force-pushed the macros/moving-repository-dir branch from 1272fa5 to b1d3a6a Compare April 9, 2024 10:31
@vincenzopalazzo
Copy link
Contributor Author

Ok I think we must complete this, I ran into some issues that are really embracing

So, if we run coffee nurse and then see the coffee remote list for a network. is the output sane?

Yes, why they should not be sane @tareknaser? I am missing something around your point?

@vincenzopalazzo
Copy link
Contributor Author

ok fixed a couple of bugs found with the nurse command.

Now the nurse command will migrate the directory, and then restore the repositories to have a fresh copy of them 😄

Review are welcome

@vincenzopalazzo
Copy link
Contributor Author

With the current code I had to run two times the nurse command to have all fixed

➜  ~ coffee nurse
╭────────────────────────────────────────────────────────────────╮
│ ●   Actions Taken                        Affected repositories │
├────────────────────────────────────────────────────────────────┤
│ ●   Moving Global repository directory   testnet               │
│ ●   Moving Global repository directory   bitcoin               │
╰────────────────────────────────────────────────────────────────╯
➜  ~ coffee remote list
Error: CoffeeError { code: 1, msg: "Coffee found some defects in the configuration. Please run `coffee nurse` to fix them.\n                    If you are want to skip the verification, please add the `--skip-verify ` flag to the command." }
➜  ~ coffee nurse
╭─────────────────────────────────────────────────────╮
│ ●   Actions Taken        Affected repositories      │
├─────────────────────────────────────────────────────┤
│ ●   Restored using Git   summars, teos, folgore-git │
╰─────────────────────────────────────────────────────╯
➜  ~ coffee remote list
● List of repositories
╭───────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ ●   Repository Alias   URL                                            N. Plugins   Git HEAD   Last Update │
├───────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ ●   folgore-git        https://github.com/coffee-tools/folgore        1            aedd703    12/03/2024  │
│ ●   summars            https://github.com/daywalker90/summars         1            c73633a    18/03/2024  │
│ ●   teos               https://github.com/vincenzopalazzo/rust-teos   1            507b8e2    09/04/2024  │
╰───────────────────────────────────────────────────────────────────────────────────────────────────────────╯

Maybe this is a way to do stuff but not sure if it is so good from implementation view point

@vincenzopalazzo
Copy link
Contributor Author

It breaks the upgrade command

➜  ~ coffee upgrade folgore-git -v
HEAD is now at 09189fb deps: changing the git url for esplora
cp: cannot stat '/home/vincent/.coffee/repositories/folgore-git': No such file or directory
✗ Error: code: 2, msg:

Copy link
Collaborator

@tareknaser tareknaser left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together. I like coffee nurse --verify output

I think the method to handle this nurse strategy should handle all the related logic so we don't need to modify patch_repository_locally_absent for example

coffee_core/src/coffee.rs Outdated Show resolved Hide resolved
coffee_core/src/coffee.rs Outdated Show resolved Hide resolved
coffee_core/src/coffee.rs Show resolved Hide resolved
coffee_core/src/coffee.rs Outdated Show resolved Hide resolved
coffee_core/src/nurse/strategy.rs Outdated Show resolved Hide resolved
coffee_lib/src/types/mod.rs Outdated Show resolved Hide resolved
coffee_core/src/nurse/strategy.rs Outdated Show resolved Hide resolved
coffee_lib/src/types/mod.rs Outdated Show resolved Hide resolved
coffee_cmd/src/coffee_term/command_show.rs Outdated Show resolved Hide resolved
coffee_core/src/coffee.rs Show resolved Hide resolved
Copy link
Collaborator

@tareknaser tareknaser left a comment

Choose a reason for hiding this comment

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

Thanks. This resolves the initial problem, but I believe there might still be a lurking bug.

coffee_cmd/src/coffee_term/command_show.rs Outdated Show resolved Hide resolved
coffee_core/src/nurse/strategy.rs Outdated Show resolved Hide resolved
@tareknaser
Copy link
Collaborator

Currently, remote repositories are still global in the coffee configuration, not specific to individual networks. To reproduce:

coffee remote add lightningd https://github.com/lightningd/plugins.git
coffee -n testnet remote add folgore https://github.com/coffee-tools/folgore.git 
coffee remote list 
coffee -n testnet remote list
coffee install folgore # Shouldn't work
coffee remote rm folgore
coffee -n testnet remote list

Expected

Output for coffee remote list: lightningd
Output for coffee -n testnet remote list: folgore

Output

tareknasser@Tareks-MacBook ~ % coffee remote add lightningd https://github.com/lightningd/plugins.git
✓ Remote added!
tareknasser@Tareks-MacBook ~ % coffee -n testnet remote add folgore https://github.com/coffee-tools/folgore.git 
✓ Remote added!
tareknasser@Tareks-MacBook ~ % coffee remote list
● List of repositories
╭──────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ ●   Repository Alias   URL                                       N. Plugins   Git HEAD   Last Update │
├──────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ ●   folgore            https://github.com/coffee-tools/folgore   1            09189fb    20/03/2024  │
│ ●   lightningd         https://github.com/lightningd/plugins     19           fef7ded    12/04/2024  │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────╯
tareknasser@Tareks-MacBook ~ % coffee -n testnet remote list
● List of repositories
╭──────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ ●   Repository Alias   URL                                       N. Plugins   Git HEAD   Last Update │
├──────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ ●   lightningd         https://github.com/lightningd/plugins     19           fef7ded    12/04/2024  │
│ ●   folgore            https://github.com/coffee-tools/folgore   1            09189fb    20/03/2024  │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────╯
tareknasser@Tareks-MacBook ~ % coffee install folgore                 
✓ Compiling and installing
✓ Plugin folgore Compiled and Installed
tareknasser@Tareks-MacBook ~ % coffee remote rm folgore
✓ Remote removed!
tareknasser@Tareks-MacBook ~ % coffee -n testnet remote list
● List of repositories
╭────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ ●   Repository Alias   URL                                     N. Plugins   Git HEAD   Last Update │
├────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ ●   lightningd         https://github.com/lightningd/plugins   19           fef7ded    12/04/2024  │
╰────────────────────────────────────────────────────────────────────────────────────────────────────╯

@vincenzopalazzo
Copy link
Contributor Author

I think your case can be written in some integration testing that we have, so looking in doing it

This is the more sane thing to do because each network
can have a different repository version.

Link: #234
Signed-off-by: Vincenzo Palazzo <[email protected]>
@vincenzopalazzo vincenzopalazzo force-pushed the macros/moving-repository-dir branch from 1cc6eb5 to 99633ee Compare April 14, 2024 16:56
@vincenzopalazzo
Copy link
Contributor Author

Thanks for this amazing catch @tareknaser

I decided to fix our tests and write your example case in our test suite, to run it, run just make integration ARGS="-- --test-threads=4 test_migrated_repository_to_local_one"

P.S: pushed the test in a single single commit, and I find also a bug inside the nurse strategy that was not checking if there was the root repositories dir.

So now we should be ok

Copy link
Collaborator

@tareknaser tareknaser left a comment

Choose a reason for hiding this comment

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

Thanks! Everything works as expected
I really like test_migrated_repository_to_local_one as well

Glad to see this one through

coffee_core/src/coffee.rs Show resolved Hide resolved
coffee_cmd/src/coffee_term/command_show.rs Outdated Show resolved Hide resolved

#[async_trait]
impl Handler for CoffeeRepositoryDirCleanUpStrategy {
async fn can_be_applied(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently running coffee nurse only detects that bitcoin network has the defect.
Same goes for coffee -n testnet nurse, it only detects that testnet has the the defect.
This is an issue because nurse removes the global repositories folder so running it the second time for another network won't find the global repositories folder.

The handler for CoffeeRepositoryDirCleanUpStrategy should look for all networks directories and fix them all at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right!

coffee_core/src/nurse/strategy.rs Show resolved Hide resolved
coffee_core/src/coffee.rs Outdated Show resolved Hide resolved
coffee_core/src/coffee.rs Outdated Show resolved Hide resolved
tests/src/coffee_integration_tests.rs Show resolved Hide resolved
Adding a new case for the coffe nurse command to clean up
the global repository directory and move it insied the network
subdirectory.

Signed-off-by: Vincenzo Palazzo <[email protected]>
@vincenzopalazzo vincenzopalazzo force-pushed the macros/moving-repository-dir branch from 99e6ad2 to 4528011 Compare April 15, 2024 18:55
@vincenzopalazzo
Copy link
Contributor Author

Find a bug when I try to add two times the same repo but in different network

➜  ~ coffee remote list
● List of repositories
╭──────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ ●   Repository Alias   URL                                       N. Plugins   Git HEAD   Last Update │
├──────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ ●   folgore-git        https://github.com/coffee-tools/folgore   1            09189fb    20/03/2024  │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────╯
➜  ~ coffee remote add folgore-git https://github.com/coffee-tools/folgore.git
✗ Fetch remote from https://github.com/coffee-tools/folgore.git error: Error while add remote: code: 1, msg: repository with name: folgore-git already exists
✗ Error: code: 1, msg: repository with name: folgore-git already exists

@tareknaser
Copy link
Collaborator

Find a bug when I try to add two times the same repo but in different network

Good point. We need to modify coffee.add_remote() so it checks against remotes specific to the network instead of global repositories

@vincenzopalazzo
Copy link
Contributor Author

Good point. We need to modify coffee.add_remote() so it checks against remotes specific to the network instead of global repositories

Thanks for suggesting the fix, I will try to look at this tomorrow :)

@vincenzopalazzo
Copy link
Contributor Author

MH we should push this forward!

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.

we should move the repository directory inside a network directory nurse command is not working as should
2 participants