Skip to content
This repository has been archived by the owner on Oct 30, 2018. It is now read-only.

Fix #370 #506

Merged
merged 2 commits into from
Jan 29, 2015
Merged

Fix #370 #506

merged 2 commits into from
Jan 29, 2015

Conversation

mscherer
Copy link
Contributor

Use the currently ignored and unused force parameters to explicitly say we want to push over a existing repository, following subversion convention. Fix issue #370

The default is changed from 'yes' to 'no' to follow
subversion behavior (ie, requiring explicit confirmation
to erase a existing repository). Since that was not working before
cf ansible#370 and since the option was ignored before and unused, this
should be safe to change.
@abadger
Copy link
Contributor

abadger commented Jan 23, 2015

This looks good to me except for changing the default value of force. Not sure what to do there -- force is not currently used in combination with export so changing the default to no would keep the same behaviour for export. But force is currently used when updating an already existing repository. So if we changed the default value we could cause errors for some people.

Maybe we should change the default value and just document that the default has changed? (Both in the module documentation and the ansible/ansible/ Changelog file?)

What's your opinion?

@abadger
Copy link
Contributor

abadger commented Jan 23, 2015

This should also fix #388

@mscherer
Copy link
Contributor Author

Indeed, didn't see force was used later in the code. So we could have a different default for each command ? It would be a bit hard to explain in the doc however.

@abadger
Copy link
Contributor

abadger commented Jan 26, 2015

I agree, that seems to be hard to document.

@justintaft
Copy link

+1 for leaving force to false for export.

If wanting to change the default value of force to true, add a warning message to the build process so people KNOW they should explicitly set the default, because it will change. Later on, make the actual change to true.

Alternatively, make the force field required in the future, and not have a default value.

For documentation

no for export, yes otherwise

isn't too confusing. It's better then breaking people's builds.

@abadger
Copy link
Contributor

abadger commented Jan 26, 2015

@mscherer: jimi-c, bcoca, and I talked it over and we're good to change the default of force to false as long as the change gets documented (so modify ChangeLog in the ansible/ansible repo to explain the effect on the subversion checkout functionality.) We'll try to review the other modules for the next major release and change other instances where force defaults to True to default to False as a safety measure as well.

abadger added a commit that referenced this pull request Jan 29, 2015
@abadger abadger merged commit 31a56e2 into ansible:devel Jan 29, 2015
@abadger
Copy link
Contributor

abadger commented Jan 29, 2015

Thanks @mscherer, I'vve merged this and will make sure the Changelog is updated.

Merged

Hi!

This has been merged in, and will also be included in the next major release.

If you or anyone else has any further questions, please let us know by stopping by one of the two mailing lists, as appropriate:

Because this project is very active, we're unlikely to see comments made on closed tickets, but the mailing list is a great way to ask questions, or post if you don't think this particular
issue is resolved.

Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants