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

Refactor storage layout and files #233

Closed
theseer opened this issue Apr 15, 2020 · 25 comments
Closed

Refactor storage layout and files #233

theseer opened this issue Apr 15, 2020 · 25 comments

Comments

@theseer
Copy link
Member

theseer commented Apr 15, 2020

Thanks to @MacFJA, phive now has support to generally download things from repositories requiring authentication.

As of now, auth information is stored in a seperate file (~/.phive/phive-auth.xml or {$basedir}./phive-auth.xml).

This seems redundant, as we already have a phive.xml which could - should? - contain the required entries for the later.

And, in case a second file should exist in the ~/.phive directory, the prefix phive- is redundant. Given we also plan to have a general config file, maybe we should move these things into it.

@MacFJA What do you think?

@theseer
Copy link
Member Author

theseer commented Apr 15, 2020

What I'm proposing is:

  • Move contents of the project specific ./phive-auth.xml into phive.xml
  • Create a new general config file in the ~/.phive directory
    • Move contents of ~/.phive/phive-auth.xml into that file
  • Adjust namespaces accordingly
    • Adopt XSD file(s)

@MacFJA
Copy link
Contributor

MacFJA commented Apr 15, 2020

I took the choice to create a new file to store auth data, to be able to have personal auth access in the file that can be ignore be the VCS.

For the phive- prefix, its more to be sure to avoid conflict with other tools that will have auth.xml file (which is a very generic name)

@MacFJA
Copy link
Contributor

MacFJA commented Apr 15, 2020

As for the ~/.phive/phive-auth.xml (redundancy), it's because I use the same behavior as \PharIo\Phive\PhiveXmlConfigFileLocator

@theseer
Copy link
Member Author

theseer commented Apr 15, 2020

I took the choice to create a new file to store auth data, to be able to have personal auth access in the file that can be ignore be the VCS.

Already expected that reasoning.
But why would such a file be project specific and not something to store in your general phive installation?

For the phive- prefix, its more to be sure to avoid conflict with other tools that will have auth.xml file (which is a very generic name)

In the case of the file being within the project directory, that makes perfect sense.

@theseer
Copy link
Member Author

theseer commented Apr 15, 2020

As for the ~/.phive/phive-auth.xml (redundancy), it's because I use the same behavior as \PharIo\Phive\PhiveXmlConfigFileLocator

That's a rather weak reason ;) as there's already a flag to signal which location to use.

Actually, this means we already have a "general" config file available - even though we currently do not use it from ~/.phive.

Still pondering what is best...

@MacFJA
Copy link
Contributor

MacFJA commented Apr 15, 2020

Already expected that reasoning.
But why would such a file be project specific and not something to store in your general phive installation?

It's the direction that Composer took (one local auth.json and one global). I suppose it's because developper are lazy and don't want to search where the global configuration is, or because for novices it's easier to add configuration at the project level. (Even if the laziest is to define it one times globally and don't have to ever worry about it :D )

Actually, this means we already have a "general" config file available - even though we currently do not use it from ~/.phive.

It's used with the --global flag. But not in a chain/failover/fallback behavior. It's the local phive.xml or the global. (cf. \PharIo\Phive\Factory::getPhiveXmlConfig)

@theseer
Copy link
Member Author

theseer commented Apr 15, 2020

I'll be pondering about that for a bit :)

@theseer
Copy link
Member Author

theseer commented Apr 26, 2020

I'm not a fan of cluttering a project's root directory with tool configuration files. And given planned features are going to require additional things to be configured that are best kept in individual files as well, the best choice is likely to adopt github's approach of having a configuration directory rather than a file.

So my suggestion would be to create a .phive directory in the project's root and put everything phive related into that directory.

Apart from the auth.xml, we can easily have a project specific repositories.xml this way and I have a bunch of other things in mind that might take advantage of this structure.

While we can keep support for the phive.xml as the lightweight variant we should optionally move the current phive.xml into that folder as well and rename it to phars.xml. To avoid conflicts, we should probably provide a migrate command to do that. But that's a different story.

@MacFJA
Copy link
Contributor

MacFJA commented Apr 27, 2020

So, the directories structure should be something like this ?

$HOME$ or $PROJECT$
└── .phive/
    ├── auth.xml # Credential data storage
    ├── phars.xml # List of installed phars (overall for $HOME$)
    └── repositories.xml # Phar repository (downloaded from https://phar.io/ for $HOME$)

$HOME_ONLY$
└── .phive/
    ├── _tmp_wrk/ # Temporary working directory
    ├── gpg/
    │   └── ... # GPG relative files ( Agent, pubrings, etc.)
    ├── http-cache/
    │   └── ... # Cache of domains requested by Phive
    ├── local.xml # Global user defined phar repositories
    └── phars/
        └── ... # Every downloaded Phar (that don't have been purged)

$PROJECT_ONLY$
├── phive.xml # For BC
└── phive-auth.xml # For BC

What should be the "fallback" mechanism ?

For getting the list of phars to install

  • Option 1: Read $PROJECT$/phive.xml, then add $PROJECT$/.phive/phars.xml without overriding
  • Option 2: Read $PROJECT$/.phive/phars.xml, then add $PROJECT$/phive.xml without overriding
  • Option 3: Read the first found in this order: $PROJECT$/phive.xml, $PROJECT$/.phive/phars.xml
  • Option 4: Read the first found in this order: $PROJECT$/.phive/phars.xml, $PROJECT$/phive.xml

For resolving a phar alias

  • Option 1
    • Read Console input
    • then add $PROJECT$/phive.xml without overriding (?)
    • then add $PROJECT$/.phive/phars.xml without overriding (?)
    • then add $PROJECT$/.phive/repositories.xml without overriding
    • then add $HOME$/.phive/phars.xml without overriding
    • then add $HOME$/.phive/local.xml without overriding
    • then add $HOME$/.phive/repositories.xml without overriding
  • Option 2
    • Read Console input
    • then add $PROJECT$/.phive/phars.xml without overriding (?)
    • then add $PROJECT$/.phive/repositories.xml without overriding
    • then add $PROJECT$/phive.xml without overriding (?)
    • then add $HOME$/.phive/phars.xml without overriding
    • then add $HOME$/.phive/local.xml without overriding
    • then add $HOME$/.phive/repositories.xml without overriding
  • Option 3
    • Read Console input
    • Read the first found in this order $PROJECT$/.phive/repositories.xml, $HOME$/.phive/local.xml, $HOME$/.phive/repositories.xml

For getting authentication data

  • Option 1: Read $PROJECT$/phive-auth.xml, then add $PROJECT$/.phive/auth.xml without overriding, then add $HOME$/.phive/auth.xml without overriding
  • Option 2: Read $PROJECT$/.phive/auth.xml, then add $PROJECT$/phive-auth.xml without overriding, then add $HOME$/.phive/auth.xml without overriding
  • Option 3: Read the first found in this order: $PROJECT$/phive-auth.xml, $PROJECT$/.phive/auth.xml
  • Option 4: Read the first found in this order: $PROJECT$/.phive/auth.xml, $PROJECT$/phive-auth.xml

For directories in $HOME$, I prefer the XDG/Freedesktop way to avoid dozen of .$PROGRAM$/ in my home directory:

  • $HOME$/.config/phive/ (auth.xml, repositories.xml, local.xml)
  • $HOME$/.cache/phive/ (http-cache/)
  • $HOME$/.local/share/phive/ (phars/, gpg/, phars.xml)
  • /tmp/$RAND$-phive/ (_tmp_wrk/)

But it's more a matter of preferences, and yet is not a universal solution either (like for Windows), but should be achievable for Unix-like OS.
(list of programs that follow XDG)

@theseer
Copy link
Member Author

theseer commented May 3, 2020

Structure

Yes, the structure as you listed it would probably be what I envisioned.

Still pondering if I like the XDG directory concept. It kind of feels useless: Instead of having all the .foo folder in $HOME, you now have it 3 fold on 3 subdirectories. Not sure if that is any better per se - just different.

On top, it would make using phive in CI harder: Instead of keeping just .phive you now would have to keep $HOME$/.config/phive, $HOME$/.local/share/phive and $HOME$/.cache/phive.

Order of things

  • For getting the list of phars to install: Option 4
  • For getting authentication data: Option 4
  • For resolving a phar alias section: Option 3

My current consideration is that we want to have a setup that uses either the $PROJECT/.phive folder or $PROJECT/phive.xml - never both. We can certainly add a warning if we encounter such a setup but explicitly ignore the $PROJECT/phive.xml in that case.

Not sure if we'll introduce this before 0.14.0 gets released. If so, we could decide to not have a phive-auth.xml to begin with, so no BC break.

@theseer
Copy link
Member Author

theseer commented May 3, 2020

Addition to the resolving part:

There's no need to use phars.xml or phive.xml for resolving. That's just our 'database' of sort. It's not used for resolving.

So, I believe this should work best, use a chain of responsibility for alias resolving:

  1. project specific: $PROJECT$/.phive/repositories.xml
  2. local system override: $HOME$/.phive/local.xml
  3. phar.io offical: $HOME$/.phive/repositories.xml

(CoR: Who ever finds something wins, otherwise forward to next.)

@MacFJA
Copy link
Contributor

MacFJA commented May 3, 2020

I miss one file: the global phive.xml (~/.phive/phive.xml) (used by remove, install, update, composer, status, outdated command with the --global flag).

What about its name?

  • Should we keep phive.xml ?
  • Should we rename it to phars.xml to keep consistency ? but it will conflict with the actual phars.xml
    • Maybe the current ~/.phive/phars.xml should change its name
    • Maybe global phive.xml and ~/.phive/phars.xml should be merged

MacFJA added a commit to MacFJA/phive that referenced this issue May 3, 2020
- Read configuration from .phive and current working directory (phars + auth)
- Update skel command
- Add warning in output if both configuration exist
- Add project level repositories
- Copy phive.xml to .phive/phars.xml
@theseer
Copy link
Member Author

theseer commented May 3, 2020

Good point.

We should probably not do either, because reusing a name for different purposes is going to be a pain to migrate / detect.

We could rename the ~/.phive/phars.xml to what it actually is: database.xml (or db.xml) and the phive.xml to global.xml.

@theseer
Copy link
Member Author

theseer commented May 3, 2020

or registry.xml ;)

@theseer
Copy link
Member Author

theseer commented May 3, 2020

I really would like to have this in the 0.14.0 to not have new things delieverd and then directly move things around for a point release.

@MacFJA
Copy link
Contributor

MacFJA commented May 3, 2020

I'm working on it, see #250 😃

I will add changes about ~/.phive/phars.xml EDIT: On a second thought it might take some time to do it

@theseer
Copy link
Member Author

theseer commented May 3, 2020

Already saw it. Thanks a lot!

It looks very promising, let me know when/if you consider it done.

@theseer
Copy link
Member Author

theseer commented May 3, 2020

I guess I'll also mess with the namespace of the auth file: https://phar.io/phive-auth => https://phar.io/phive/auth.

I'm still pondering on the xml structure inside it, but so far nothing I can come up with is actually better - just different. I'm not fully happy with the format but i'll open another ticket for that so we can discuss that separately.

Do we need a migrate command actually to get the files moved and converted?

@MacFJA
Copy link
Contributor

MacFJA commented May 3, 2020

I'm writing a migration service to handle those kind of action

With the idea that migration flagged as mustMigrate are run when Phive start


src/services/migration/Migration.php

<?php declare(strict_types = 1);
namespace PharIo\Phive;

interface Migration {
    /**
     * Indicate if the migration can be done.
     * Return false if the migration is already done, or no doable
     */
    public function canMigrate(): bool;

    /**
     * Indicate if we allow the state before and after at the same time (false).
     * Return true if only the new state is allowed
     */
    public function mustMigrate(): bool;
    public function migrate(): void;
}

src/services/migration/HomePharsXml.php

<?php declare(strict_types = 1);
namespace PharIo\Phive;

class HomePharsXml implements Migration {
    /** @var Environment */
    private $environment;

    public function __construct(Environment $environment) {
        $this->environment = $environment;
    }

    public function canMigrate(): bool {
        return $this->environment->getHomeDirectory()->file('phars.xml')->exists()
            && !$this->environment->getHomeDirectory()->file('database.xml')->exists();
    }

    public function mustMigrate(): bool {
        return true;
    }

    public function migrate(): void {
        if (!$this->canMigrate()) {
            throw new MigrationException();
        }

        $old = $this->environment->getHomeDirectory()->file('phars.xml');
        $new = $this->environment->getHomeDirectory()->file('database.xml');

        $new->putContent($old->read()->getContent());
        $old->delete();
    }
}

MacFJA added a commit to MacFJA/phive that referenced this issue May 3, 2020
- Change name of ~/.phive/phars.xml to ~/.phive/database.xml
- Change name of ~/.phive/phive.xml to ~/.phive/global.xml
- Add migration service
- Add migrate command
- Add auto migration of mandatory migration
@MacFJA
Copy link
Contributor

MacFJA commented May 3, 2020

let me know when/if you consider it done.

Almost done, I just need to add a lot more tests to ensure that migration don't mess up everything.
I'm open to any comment/review

MacFJA added a commit to MacFJA/phive that referenced this issue May 9, 2020
- Read configuration from .phive and current working directory (phars + auth)
- Update skel command
- Add warning in output if both configuration exist
- Add project level repositories
- Copy phive.xml to .phive/phars.xml
MacFJA added a commit to MacFJA/phive that referenced this issue May 9, 2020
- Change name of ~/.phive/phars.xml to ~/.phive/database.xml
- Change name of ~/.phive/phive.xml to ~/.phive/global.xml
- Add migration service
- Add migrate command
- Add auto migration of mandatory migration
MacFJA added a commit to MacFJA/phive that referenced this issue May 10, 2020
- Read configuration from .phive and current working directory (phars + auth)
- Update skel command
- Add warning in output if both configuration exist
- Add project level repositories
- Copy phive.xml to .phive/phars.xml
MacFJA added a commit to MacFJA/phive that referenced this issue May 10, 2020
- Change name of ~/.phive/phars.xml to ~/.phive/database.xml
- Change name of ~/.phive/phive.xml to ~/.phive/global.xml
- Add migration service
- Add migrate command
- Add auto migration of mandatory migration
theseer pushed a commit that referenced this issue May 10, 2020
- Read configuration from .phive and current working directory (phars + auth)
- Update skel command
- Add warning in output if both configuration exist
- Add project level repositories
- Copy phive.xml to .phive/phars.xml
theseer pushed a commit that referenced this issue May 10, 2020
- Change name of ~/.phive/phars.xml to ~/.phive/database.xml
- Change name of ~/.phive/phive.xml to ~/.phive/global.xml
- Add migration service
- Add migrate command
- Add auto migration of mandatory migration
@theseer
Copy link
Member Author

theseer commented May 10, 2020

Thanks a lot @MacFJA !

@theseer theseer closed this as completed May 10, 2020
@theseer
Copy link
Member Author

theseer commented May 11, 2020

Okay, ran some more tests and looked a things. Of course, we have a few issues left. Some of these I should have seen earlier, sorry about that.

.phive project directory

The phar-io/filesystem's Directory class enforces existence. That isn't a new problem (it's already a problem for the tools directory) but now causes an empty $project/.phive directory to be created on every run. Time to fix that. I'll open a ticket on phar-io/filesystem tomorrow.

Migrations

We do run too many things optionally and expose them to the user.

The migrations for the files in ~/.phive/ are an internal change. As it's not user facing it's neither optional nor something to be done on demand. We just do it. No backup once the new file(s) are written. No status.

I'm not sure we do need the Auth migration, actually. Your first implementation was never released. I'd say, we can just remove it?

In the command we should probably "only" move the phive.xml file into the .phive folder. Or am I missing anything thing that needs migration as of now?

Status output

The 'Status' output is wrong. It claims 'Migrated' for stuff that doesn't exist. For instance, I don't have an auth.xml file in this project, so how could it be migrated?

@theseer theseer reopened this May 11, 2020
@theseer theseer changed the title Reconsider storage location for auth data Refactor storage layout and files May 11, 2020
@theseer
Copy link
Member Author

theseer commented May 11, 2020

Additional thing: We probably should inform the user what we are migrating and ask for confirmation before doing it. :)

@MacFJA
Copy link
Contributor

MacFJA commented May 11, 2020

.phive project directory

The phar-io/filesystem's Directory class enforces existence. That isn't a new problem (it's already a problem for the tools directory) but now causes an empty $project/.phive directory to be created on every run. Time to fix that. I'll open a ticket on phar-io/filesystem tomorrow.

Yeah, I have this issue too, that why I wrote test like

$workingDirectory->hasChild('.phive') && $workingDirectory->child('.phive')->file('phars.xml')->exists();

But it had lots of code/logics.


I'm not sure we do need the Auth migration, actually. Your first implementation was never released. I'd say, we can just remove it?

I wasn't either sure if it was really needed or not


The migrations for the files in ~/.phive/ are an internal change. As it's not user facing it's neither optional nor something to be done on demand. We just do it. No backup once the new file(s) are written. No status.

Make sense, and easy to correct


Status output

The 'Status' output is wrong. It claims 'Migrated' for stuff that doesn't exist. For instance, I don't have an auth.xml file in this project, so how could it be migrated?

Hmm... maybe Nothing to do is a better status ?

@theseer
Copy link
Member Author

theseer commented May 11, 2020

.phive project directory

The phar-io/filesystem's Directory class enforces existence. That isn't a new problem (it's already a problem for the tools directory) but now causes an empty $project/.phive directory to be created on every run. Time to fix that. I'll open a ticket on phar-io/filesystem tomorrow.

Yeah, I have this issue too, that why I wrote test like

$workingDirectory->hasChild('.phive') && $workingDirectory->child('.phive')->file('phars.xml')->exists();

But it had lots of code/logics.

Yeah, I know. I'll take care of it will also solve issue #179.

I'm not sure we do need the Auth migration, actually. Your first implementation was never released. I'd say, we can just remove it?

I wasn't either sure if it was really needed or not

Let's drop it then.

The migrations for the files in ~/.phive/ are an internal change. As it's not user facing it's neither optional nor something to be done on demand. We just do it. No backup once the new file(s) are written. No status.

Make sense, and easy to correct

Okay, can yo do that?

Status output

The 'Status' output is wrong. It claims 'Migrated' for stuff that doesn't exist. For instance, I don't have an auth.xml file in this project, so how could it be migrated?

Hmm... maybe Nothing to do is a better status ?

How about simply N/A? Or just not showing it at all if the file's required for the migration to be run aren't present?

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

No branches or pull requests

2 participants