Skip to content

Add option to not process existing workspace as error #137

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

Merged
merged 2 commits into from
Nov 14, 2014

Conversation

webda2l
Copy link

@webda2l webda2l commented Nov 12, 2014

silent-on-exist or better if you prefer

@dbu
Copy link
Member

dbu commented Nov 12, 2014

@dantleech wdyt? i agree its a good idea to do something here.

we might just output a warning but return state 0, for automatic deployments. or a -f / --force parameter, like with rm?

@dantleech
Copy link
Member

Hmm. I tend to do the following in these situations:

php app/console doctrine:phpcr:workspace create foobar &> /dev/null

But yeah, would be safer with a flag. Maybe it would be better to talk about idempotency: --idempotent ? Are there any similar flags in other commands in the world?

btw. Doctrine does not support this with database:create.

@dbu
Copy link
Member

dbu commented Nov 12, 2014

the rm command uses -f - when you force, it does not complain if the target file does not exist. from the use case of this command, i would either do -f or even just stop doing a non-0 exit state and have a --fail-if-exists option.

@dantleech
Copy link
Member

But force in the case of rm is to force the removal. In this case it doesn't make sense, unless we are "forcing" the command not to throw an exception.

I think "idempotent" represents what we are doing well. If it doesn't exist, create it, if it does exist, do not do anything. This could also apply to other operations like workspace:remove, node:create etc.

@webda2l
Copy link
Author

webda2l commented Nov 12, 2014

As you want. indempotent is not very common, but..!
I use "&> /dev/null" tips currently, thanks.

@dbu
Copy link
Member

dbu commented Nov 14, 2014

Idempotent is very technical. What about ignore-existing (short i)? Or just print a warning but keep 0 exit state without any option?

@dantleech
Copy link
Member

I think --ignore-existing is good (but I think we should not have a shortcut (yet) -- it would likely clash with other shortcuts as we should use this option for all commands with similar requirements).

@dbu
Copy link
Member

dbu commented Nov 14, 2014

@dantleech but you think there is worth in keeping this returning a non 0 status at all?

@dantleech
Copy link
Member

Yeah, I think we should return non zero if the --ignore-existing flag is not used.

Some examples of other things:

Mysqladmin:

$ mysqladmin create test; echo "Exit code: "$?                                                                                                                                                                                    node-validator a7494d2 ✗
mysqladmin: CREATE DATABASE failed; error: 'Can't create database 'test'; database exists'
Exit code: 255

Doctrine:

$ ./app/console doctrine:database:create; echo "Exit code: "$?                                                                                                                                                               docker 76b0051 ✗
Could not create database for connection named `dtlweb1`
An exception occurred while executing 'CREATE DATABASE `dtlweb1`':

SQLSTATE[HY000]: General error: 1007 Can't create database 'dtlweb1'; database exists
Exit code: 1

Postgres

psql -Upostgres -c "create database sulutest"; echo "Exist code: "$?                                                                                                                                                       docker 76b0051 ✗
ERROR:  database "sulutest" already exists
Exist code: 1

@dbu
Copy link
Member

dbu commented Nov 14, 2014

okay, convinced. i see the code is already updated. thanks a lot.

dbu added a commit that referenced this pull request Nov 14, 2014
Add option to not process existing workspace as error
@dbu dbu merged commit 72e8865 into phpcr:master Nov 14, 2014
@dbu
Copy link
Member

dbu commented Nov 14, 2014

the build fail is a hhvm issue with a test taking more than 1 second which has nothing to do with this PR

@webda2l
Copy link
Author

webda2l commented Nov 17, 2014

Thanks! As previously, a minor release will be nice.

@webda2l webda2l deleted the create-workspace branch November 17, 2014 09:18
@dbu
Copy link
Member

dbu commented Nov 17, 2014

i would love to fix the hhvm fail before the release. anybody got a quick idea how to tell phpunit that its ok if a test takes > 1 second?

@dantleech
Copy link
Member

You can use the @small, @medium and @large annotations: https://phpunit.de/manual/current/en/appendixes.annotations.html#appendixes.annotations.medium

@dbu
Copy link
Member

dbu commented Nov 17, 2014

there you go, just tagged 1.2.2

thanks a lot!

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.

3 participants