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

Per user persistent storage #1269

Merged

Conversation

ccristescu-reol
Copy link
Contributor

Q A
Bug fix? No
New feature? Yes
BC breaks? No
Deprecations? No
Fixed tickets N/A

In a multi user environment where multiple projects are located on the same instance(s)
if two or more users are deploying a project on the same environment, the generated hosts files
in the system tmp folder are overwritten or worse, the deploy will fail due to file permissions.

Make the persistent storage folder configurable or use the system tmp as default if not possible.

…e same instance(s)

	if two or more users are deploying a project on the same environment, the generated hosts files
	are overwritten or worse, the deploy will fail due to file permissions

Make the persistent storage folder configurable or use the system tmp 
	as default if not possible
when the persistent storage it's located in the system tmp
when the persistent storage it's located in the system tmp.

Dropped the uniqueid.
// use the system temporary folder and the current pid as default
// persistent storage in case we can't use the configured value
// or we can't create the default location
$tmp = sys_get_temp_dir() . '/' . posix_getpid();
Copy link
Member

Choose a reason for hiding this comment

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

This extension is not available on Windows platforms.
http://php.net/manual/en/intro.posix.php

Copy link
Contributor Author

@ccristescu-reol ccristescu-reol Jun 19, 2017

Choose a reason for hiding this comment

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

For non posix compatible i'm going to use getmypid.

* @return string
* @throws Exception
*/
private static function _getPersistentStorageLocation() {
Copy link
Member

Choose a reason for hiding this comment

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

Rename _getPersistentStorageLocation to getPersistentStorageLocation and move this method to end of class.

$tmp = sys_get_temp_dir() . '/' . posix_getpid();

// get the location for the deployer configuration
$deployerConfig = $config->has('deployer_config') ? $config->get('deployer_config') : null;
Copy link
Member

Choose a reason for hiding this comment

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

It's over this PR, I think such functionality should done in separate PR.

Anyway I don't see way it's needed. Please remove everything related to deployer_config.


// use the home dir of the current user
// and the repository name
$userInfo = posix_getpwuid(posix_getuid());
Copy link
Member

Choose a reason for hiding this comment

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

Same here, what about windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For non posix compatible i'm going to use getenv.
getenv('HOMEDRIVE'), getenv('HOMEPATH') are available on windows 8+
getenv('HOME') for *nix

It's Windows 8+ enough?

}

// we now have the repository name
$repository = str_replace('/', '_', substr($configRepository, (strrpos($configRepository, ':') + 1)));
Copy link
Member

Choose a reason for hiding this comment

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

What is it for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Long story short: i have five webapps on the same instances (my staging). If i deploy two apps at the same time (using two terminals for example) the second one i launch will overwrite the content of the .dep files so, the first deployer will start running tasks in the locations set by the second one and everything ends up in a mess.

The .dep files should be stored per webapp so, the easiest way was to use the repository to generate a separate folder name.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's easier to create rundom files instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Easier yes... but then you will need an internal task to clean up temporary files older than N days or something simmilar.
This way it's cleaner... i would say :)

Copy link
Member

Choose a reason for hiding this comment

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

But tmp cleaned by system itself

Removed the option to used a configuration variable to set the persistent storage location.
@ccristescu-reol
Copy link
Contributor Author

Windows 8+ and non posix systems compatibility, added alternatives to the posix_* calls:

  • getmypid() for posix_getpid
  • getenv()for posix_getpwuid

Method _getPersistentStorageLocation renamed to getPersistentStorageLocation and moved at then end.

Dropped the option to have a configurable path for the persistent storage folder.

}

// we now have the repository name
$repository = str_replace('/', '_', substr($configRepository, (strrpos($configRepository, ':') + 1)));
Copy link
Member

Choose a reason for hiding this comment

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

I think it's easier to create rundom files instead.

@ccristescu-reol
Copy link
Contributor Author

Removed everything and replaced it with randomly generate file names in system tmp.

@antonmedv antonmedv merged commit ab75afa into deployphp:master Jul 3, 2017
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.

2 participants