mercurial module needs review and feedback!

https://github.com/yeukhon/ansible-mercurial

The early hg module was barely usable to me so I have to write one myself.

Tests:

  1. currently using it myself

  2. ssh, https, private and public repos

  3. doc is cleaned up suggested by Michael here .

  4. more sane checkpoints in clone

  5. fixed some bugs

  6. cleaned up some bit of the code (including a bit nicer error display).

Please help review and test if possible!
You can always just symlink this module to ur library.

Example:

I missed the early hg module, was it Brad Olson’s?
https://github.com/bradobro/ansible-module-mercurial

I see that there are some differences:

  • yeukhon internals nicely separates arguments into lists, perhaps cleaner function paths?
  • bradobro supports more arguments: revision, owner, (and clean if Brad would pull from roller)
  • bradobro returns changed false if repo id is unchanged

I am using bradobro one for my stuff, but I there are no tests provided.
-joel

The other module didn't (IIRC) use the new Python module
infrastructure, so it's not possible to include in core for maintaince
reasons.

I don't know what owner did (not looking ATM), but we can use file
attributes on any module regardless now.

Other things do seem like they should be fixed up.

The other module didn’t (IIRC) use the new Python module
infrastructure, so it’s not possible to include in core for maintaince
reasons.

Sure, it’d be nice to have a mercurial module in core. It actually does look like bradobro has done some updates re AnsibleModule() and not bothered to announce them.

I don’t know what owner did (not looking ATM), but we can use file
attributes on any module regardless now.

bradobro’s owner argument used su to run mercurial as that user. This was very convenient, allowing me to create a deployment user with generate_ssh_key, and then fetch those keys to the repository host. I’ve had issues in the past with ssh agent forwarding for this purpose.

Perhaps that’s a fairly overloaded use of the word owner though, using file module for actual ownership changing sounds interesting.

I don't know what owner did (not looking ATM), but we can use file
attributes on any module regardless now.

bradobro's owner argument used su to run mercurial as that user. This was
very convenient, allowing me to create a deployment user with
generate_ssh_key, and then fetch those keys to the repository host. I've
had issues in the past with ssh agent forwarding for this purpose.

Using su there is not acceptable -- modules should never do that, and
there's no reason to assume someone was running as root.

Rather, the task should specify the sudo_user, or the play should just
use that user directly.

All of this exists in core ansible.

Here is my thought.
I’d avoid using sudo, sudo_user whenever possible. The core should be simple.
I have to use ssh to deployment form private ssh repo on bitbucket so I have ran into cases when root doesn’t have access to ~/.ssh/config

I am proposing three changes to the existing module.

  1. change mercurial to hg (as you can see, I was developing it on top of the old code, erased nearly everything, but forgot to rename it).
  2. append StrictHostKeyChecking=no to ssh if the user chooses to (meaning new option)

To accept key without prompt, we can disable the prompt by specifying StrictHostKeyChecking=no in the config file or in the ssh command.
This first goes through the check, and then accept even if no match is found in known hosts.
I don’t know the safest method. But if one requires a strict key checking, ssh should check against a list of approved known hosts managed by ansible as part of the configuration, or simply accept it

  1. rev to revision
    I went through git and this old module again and i find they both use revision rather than calling it rev.
    I will also try to make sure attributes are the same across all modules.

There are still some refactoring to be done in my hg module.

I am not trying to take away his credit (I mention it in my first line of README), except, to be in the core, and to be a good open source project, active development is required.

Here is my thought.
I’d avoid using sudo, sudo_user whenever possible. The core should be simple.

I actually missed the 0.8 change that allows you to put sudo_user in any task. That certainly does make everything simpler.

  1. rev to revision
    I went through git and this old module again and i find they both use revision rather than calling it rev.
    I will also try to make sure attributes are the same across all modules.

This is option is missing from the doc block. (Though it is in the example)

I am not trying to take away his credit (I mention it in my first line of README), except, to be in the core, and to be a good open source project, active development is required.

Sorry, I missed this, too. I guess the code was different enough that I assumed an entirely different source. I’d request a “clean” or “force” update feature as well, if you’d like I can submit a patch later. Also, it may be nice to alias the git modules options where possible.

-joel

Hi Joe.

Sorry, I missed this, too. I guess the code was different enough that I assumed an entirely different source
It’s good, no sorry. I like to make sure everything is all right before saying GOOD. That’s why I put guards (hg verify and hg tip) as much as possible.

I’d request a “clean” or “force” update feature as well, if you’d like I can submit a patch later. Also, it may be nice to alias the git modules options where possible.
I see the usage for local development, and the use after manually fixing something on a production machine and wishing to discard them in the next deployment/configuration cycle.
But please educate me the common usage, espeically in an auto-deployment. We can add them since git and svn have them. I just want to know it before it goes to the code.

if you’d like I can submit a patch later. Also, it may be nice to alias the git modules options where possible.
Yes. Of course. That’s how we learn. As a student open source helps me grow. As a user, that’s the heart of Open source :slight_smile:

Cheer,
Yeukhon

I use this specifically for development. Auto-deployment is great for production, but it’s also really nice to bootstrap a dev environment. I use an clean_update=no inventory variable on dev machines, that way I can test everything including ansible deploy before the commit.

The most extreme case being a task that updates your ansible plays, you may want to test it without losing your work.

-joel

That’s actually a nice idea. I’d like to include that, regardless :smiley:
A patch is welcome! Make a pull request :slight_smile:

Thanks!

So I finally have a little more time to work on this. Been really busy this whole weekend with other stuff.

I think to do cleanup, one might want

hg up -C to discard uncommitted changes
then hg purge

However, purge (or clean) is not enabled by default, which requires enabling it in hgrc file. There is --config but it never works out for me. So if we go with purge, we have to (1) make a backup of the current hgrc file, then (2) edit the hgrc file and finally (3) restore the old hgrc file

So the option is called force which will discard modified files, and untracked files.

As far as I’m concerned, the mercurial module was a dirty hack I never thought would make it into core because Michael thought people should just switch to git. (I do to, but, alas, have to deploy some things from Mercurial.) I made it for myself, the way I liked it, and didn’t know many others were using it.

To set the record straight:

  1. I’m totally happy to have someone come up with a better module or take over maintenance of this one. Yeukhon, if you can build it better, go for it! I haven’t looked at your code yet, but plan to. Thanks for the contribution. If you want to collaborate, I’m open. I’d love to see the module in core, but have no interest in arguing about it with Michael.

  2. I have to deploy code as a particular use. For the same reason the file module has to specify an owner, I have to specify a different user of a repo. Sudo is the only way I see to do that, but I’m open to suggestions.

  3. Michael wrote, "My module uses #<<INCLUDE_ANSIBLE_MODULE_COMMON>>, but if there are new methods, I haven’t kept up.

  4. Michael writes, “I don’t know what owner did (not looking ATM), but we can use file attributes on any module regardless now.” Not sure what that means, so there must be some developments I haven’t kept up with. If you point me to the docs or explain a bit, I can try adjust it.

  5. Sorry I missed a patch from roller. I always clean, but I can see why you might want it as an option. I’ll check my github messages. Thanks for the heads up, joel.

  6. Yeah, no tests. I need some. New responsibilities and not enough bandwidth.

Nice code, Yeukhon!

Hi. Thanks for the encouragement.
I think we all hg users can help build it. I don’t know every great commands in hg. There are certainly shortcuts and improvements can be made. So I am totally opened to working with everyone! Just educate me if you guys have spare time :smiley:

I looked at all the existing modules, they all use <<INCLUDE_ANSIBLE_MODULE_COMMON>>

re to #4, I think he’s saying you can set use the file module to set owner, group and mode permission. With sudo_user you perform su-like operation.

I have tests, but I need those tests to roll genetic. I don’t know any other good ways to run tests besides rolling up a vagrant vm.

I am going to finsih the force option tomorrow night if what I suggested above makes sense.
basically, hg update -C to discard modified changes, and then hg purge (which requires enable the extensions in hgrc temporarily).

Yeukhon

As far as I'm concerned, the mercurial module was a dirty hack I never
thought would make it into core because Michael thought people should just
switch to git. (I do to, but, alas, have to deploy some things from
Mercurial.) I made it for myself, the way I liked it, and didn't know many
others were using it.

There's enough of an audience now to where having more SCM types is
starting to make sense.

Though everyone should pick git :slight_smile:

To set the record straight:

1. I'm totally happy to have someone come up with a better module or take
over maintenance of this one. Yeukhon, if you can build it better, go for
it! I haven't looked at your code yet, but plan to. Thanks for the
contribution. If you want to collaborate, I'm open. I'd love to see the
module in core, but have no interest in arguing about it with Michael.

2. I have to deploy code as a particular use. For the same reason the file
module has to specify an owner, I have to specify a different user of a
repo. Sudo is the only way I see to do that, but I'm open to suggestions.

3. Michael wrote, "My module uses #<<INCLUDE_ANSIBLE_MODULE_COMMON>>, but if
there are new methods, I haven't kept up.

The new 'run_command' is very recent in 1.0.

4. Michael writes, "I don't know what owner did (not looking ATM), but we
can use file attributes on any module regardless now." Not sure what that
means, so there must be some developments I haven't kept up with. If you
point me to the docs or explain a bit, I can try adjust it.

Whatever the owner= flag did, if it was doing chmod's and such, it
should not need to.
But I wasn't looking.

5. Sorry I missed a patch from roller. I always clean, but I can see why you
might want it as an option. I'll check my github messages. Thanks for the
heads up, joel.

6. Yeah, no tests. I need some. New responsibilities and not enough
bandwidth.

I don't think we really need automated tests of hg checkouts that
badly -- so I'm fine with this just being tested
when people work on it. It won't change much.

That being said, the git module has had a fair amount of turnover.

Thanks for the input, Michael. +1 on git, and thanks for the run_command note.

Joel & Yeukhon,

My work schedule won’t allow me much bandwidth for this until after Feb 20, but I’m very interested in seeing a stable hg module.

Do either of you have energy now to lead this module?

Yeukhon, could you articulate what made the old module unusable for you? Conversely, are there any options you’d like added to your module?

Joel, you probably have both modules in your head more, plus you’ve written the spec docs. Can you propose a unifying path forward on this?

Sure. I can lead the module. Anyone can help.
As far as tests goes, yeah, I haven’t dived into ansible’s tests yet. Not sure exactly how they test it.

I can’t remember the exact problems, but it threw me errors. Even reading the code I wasn’t able to correct it. Given time cosntraints, I decided to just write one up by calling hg commands.

I think the biggest issue I have with my own implemtnation is all the health check points. Maybe too much for big repos but if it’s automated they seem logical to have.
Also user feedback. Whether they find it useful or not. I will roll out the changes later.

John

The repo has been updated with force option.
https://github.com/yeukhon/ansible-mercurial

It’s now in the development branch.
https://github.com/ansible/ansible/pull/1922

Thanks for all the feedback and help.

Yeukhon

Thank you all for getting this all together. I haven’t had a chance to check out the new module yet, but I’ll try to verify my use cases this weekend.

A note about the force option: I find the purge behavior a bit surprising. This goes pretty far beyond what default hg clean updates do. Considering the feature involves enabling a module, it may be something left as a separate option rather than the default. It seems like it’d be a nice feature, and I like hack temporarily enabling extra modules to get it, but it seems like something that should not be the default.

Congrats on going core, too. :slight_smile: If I want to propose patches at this point, would I be right to branch ansible/ansible rather than yeukhon/ansible-mercurial?

-joel