bittner commented 7 years ago

Adds two parameters to the software::vcsscm::git class:

gitconfig (false): Add both a system-global and a user-global .git/config file to the host
gitignore (false): Add a user-global .git/ignore file to the host

Fixes #8.

Envisioned Concept

Currently, when one of those parameters is set to true the files are installed based on the idea of "sensible defaults". They should be generally suitable, practically for "everyone". But not worthless, i.e. empty files.

If future, the parameters may allow to specify a Puppet URL ('puppet:///modules/...) or something similar in order to override the files provided by the software Puppet module. For the gitconfig parameter that would entail to handle 4 cases: (Examples)

Edge Cases

Create only a user-global gitconfig file (no system-global):
```
gitconfig => 'puppet://module/...'
```

2.-4. Create both files, or only one of them:

  gitconfig => {
    'system' => 'puppet:///module/...',
    'user'   => 'puppet:///module/...',
  }

Case 1. would hence (have to) be equivalent to:

  gitconfig => {
    'user'   => 'puppet:///module/...',
  }

The non-specified files would have to be removed (by default) to leave the host in a predictable state.

bittner commented 7 years ago

Would be cool if you could merge this. We can then work on enhancing the solution afterwards, incrementally. I think that wouldn't hurt.

bittner commented 6 years ago

Anything I can do to make you merge this PR?

We've got a Sprint goal that requires us to set up client PCs for development, and this addition (plus the one related to Slack) would help substantially.

edestecd commented 6 years ago

Sorry I just got back from vacation and i'm still trying to catch up at work. I'll try to get back to this today or tomorrow.

edestecd commented 6 years ago

I'm not a fan of the hardcoded config files. There is no opportunity to modify these and it seems useful in your environment only. The module should be useful to anyone that wants to use it. Custom configurations are the job of profiles or hiera.

I'm also not sure about the /etc/skel stuff. This seems like maybe a skel module should mange it. OR at least a skel class... It seems more related to skel or users than git.

I would accept something like this:

#params
$system_gitconfig_source   = undef,
$skel_gitconfig_source      = undef,
$skel_gitignore_source      = undef,

...

     if $system_gitconfig_source {
        file { '/etc/gitconfig':
          ensure => file,
          owner  => 'root',
          group  => 'root',
          mode   => '0644',
          source => $system_gitconfig_source,
        }
     }

      if $skel_gitconfig_source {
        file { '/etc/skel/.config/git/config':
          ensure => file,
          owner  => 'root',
          group  => 'root',
          mode   => '0644',
          source => $skel_gitconfig_source,
        }
      }

      if $skel_gitignore_source {
        file { '/etc/skel/.config/git/ignore':
          ensure => file,
          owner  => 'root',
          group  => 'root',
          mode   => '0644',
          source => $skel_gitignore_source,
        }
      }

This way anyone can specify their preferred config. I still question the skel stuff a bit, but I'd accept it....

bittner commented 6 years ago

This seems like maybe a skel module should mange it.

I guess one could argue that this is about configuring Git on (developer) machines, which is a topic per se. It overlaps with skel, but it really has a different meaning, it's like viewing things from a different angle (the Git user angle instead of the administrator or system user angle).

it seems useful in your environment only

I thought about this, too. There are quite a few common issues you may also have noticed when working with Git. How often did you do a research (hoping for an answer on StackOverflow to give you a solution) for git commit --undo or git merge --undo or git rebase --undo or something similar?

undo-commit = reset --soft HEAD^

... addresses one common theme, maybe even the most common one. Combined with the Bash autocompletion this is great for typing $ git undo<tab> in a moment of desperation.

As of the global .gitignore, this is also a common thing, and there are loads of repositories out there that suggest default setups for several technologies and technology stacks (with GitHub's gitignore being one of the most well-known, probably). Gitignore.io is a nice tools that generates them even for combined technologies. This is not something I have invented myself. It's a need of the market.

Biased But Useful

Yes, all in all it is a biased solution, but with the option of overriding this biased default setup -- why not provide a convenience solution?

Let's throw out the stuff you find awkward, and keep useful stuff in the setup. This could even make this Puppet module popular, when it turns out to be particularly helpful, out of the box.

edestecd commented 6 years ago

The one I saw that seemed akward was the editor = emacs. Not sure this is even a good default. Is emacs even installed by default on Ubuntu?

edestecd commented 6 years ago

I suppose I'm not against any of the other settings as defaults, but I would at least like an option to override or append them. A template with a loop on a hash at the end of the defaults or the ability to change the file source would be good.

bittner commented 6 years ago

The emacs thing is indeed a home-grown beast from our office. I'll remove that and try to find a better, generic solution. As you say, a template with a loop on a hash at the end of the defaults or the ability to change the file source would be good. I'll look into this.

edestecd commented 6 years ago

If you have time, a few more scenarios in here, for the added options, would be nice: https://github.com/edestecd/puppet-software/blob/master/spec/classes/software_vcsscm_git_spec.rb

bittner commented 6 years ago

You're right, that is one thing I wanted to mention:

I feel ashamed I'm having a hard time adding tests. This is actually quite inconvenient for development, because I have to try the module out by doing a live installation.

Can you give me a hand writing tests? I thought to cover the following scenarios:

1. Booleans:

  class { 'software::vcsscm::git':
    ensure          => present,
    gui             => true,
    bash_completion => true,
    bash_prompt     => true,
    gitconfig       => true,
    gitignore       => true,
  }

2. Strings:

  class { 'software::vcsscm::git':
    ensure          => present,
    gui             => true,
    bash_completion => true,
    bash_prompt     => true,
    gitconfig       => 'puppet://...',
    gitignore       => 'puppet://...',
  }

3. ~~Associative arrays:~~ Hashes:

  class { 'software::vcsscm::git':
    ensure          => present,
    gui             => true,
    bash_completion => true,
    bash_prompt     => true,
    gitconfig       => {
      'system' => 'puppet://...',
      'user'   => 'puppet://...',
    },
    gitignore       =>  {
      'user'   => 'puppet://...',
    },
  }

bittner commented 6 years ago

BTW, the linting complains about missing trailing commas, but I don't see that this complaint is correct.

bittner commented 6 years ago

I've rebased the branch onto the new master/HEAD.

I don't think I can provide more scenarios for the test specs w/o guidance, I'm afraid.

bittner commented 6 years ago

It was repeated in another module.

/etc/skeleton/.config ($HOME/.config, really) is a common place for software to put their configuration files in. However, file { ensure => directory } fails with mkdir unless you ensure the base path exists.

From the documentation I understand that ensure_resource is problematic too, because it only works when all resource parameters are absolutely identical (matching the full base path may not work for all possible conflicts).

bittner commented 6 years ago

@edestecd What do I have to do to get this merged?

edestecd commented 6 years ago

According to puppets architecture, we should have a separate module that manages the common dir and both modules should depend on it. That is the official way to handle this. Many choose to do it anyways.

This is a pattern I have added to many modules to fix it. execs are really frowned upon. They have their place, but I really don't like to use them for file operations, if I can avoid it.

https://github.com/biemond/biemond-oradb/blob/7827253b2bfe09936ced29936cb1997e3e2d9228/manifests/installem_agent.pp#L151

The !defined solves some issues with resource params needing to be identical, but it must come after the other declaration in code execution... If all else fails the $manage_ param can be used to turn it off in one of the modules...

edestecd commented 6 years ago

If you switch your code to these case statements the linter passes. I tested it. Not sure why the other if syntax is not passing the linter. The doc claims both should work: https://puppet.com/docs/puppet/5.3/lang_data_type.html#cases

At any rate, I think the cases look cleaner and are actually less lines of code... These also fail if the value is an unexpected type.

      if $gitconfig {

        $gitconfig_system = $gitconfig ? {
          Boolean => 'puppet:///modules/software/vcsscm/git/system-gitconfig',
          String  => false,
          Hash    => $gitconfig['system'],
          default => fail('$gitconfig must be one of (Boolean, String, Hash)'),
        }
        $gitconfig_user = $gitconfig ? {
          Boolean => 'puppet:///modules/software/vcsscm/git/user-gitconfig',
          String  => $gitconfig,
          Hash    => $gitconfig['user'],
          default => fail('$gitconfig must be one of (Boolean, String, Hash)'),
        }

      if $gitignore {

        $gitignore_user = $gitignore ? {
          Boolean => 'puppet:///modules/software/vcsscm/git/user-gitignore',
          String  => $gitignore,
          Hash    => $gitignore['user'],
          default => fail('$gitignore must be one of (Boolean, String, Hash)'),
        }

edestecd commented 6 years ago

Let me get you some spec examples that set params and check for the proper resources to be present. Here are some examples: https://github.com/edestecd/puppet-clamav/blob/master/spec/classes/clamav_spec.rb#L29 https://github.com/puppetlabs/puppetlabs-apache/pull/1687/files

You just need a few new context blocks here: https://github.com/edestecd/puppet-software/blob/master/spec/classes/software_vcsscm_git_spec.rb#L10

example: "with booleans for gitconfig and gitignore"

One for each of the scenarios you mentioned above.

edestecd commented 6 years ago

Seems like that should do it. Sorry for the delay. I just got back from puppetconf... I'm trying to not let spec tests slide any longer. This module needs some serious spec test love...

edestecd commented 6 years ago

@bittner looks good to me. Just need a few more examples here: https://github.com/edestecd/puppet-software/blob/master/spec/classes/software_vcsscm_git_spec.rb#L10

Let me know if you need any help on those...

bittner commented 6 years ago

That feels better now with RSpec acceptance tests. I only covered the sunshine scenarios, though.

For some reason rubocop complains about the final end in the RSpec file. Not sure what's happening. I don't see any mistake syntax-wise.

edestecd commented 6 years ago

The scenarios you have are enough.

bittner commented 6 years ago

This PR really wants to know how far we are willing to go!

Ruby is satisfied, but there is now an Unknown resource type: 'vcsrepo' for some reason. The related Puppet module is installed, however.

Can you spot what's broken?

edestecd commented 6 years ago

You need to add vcsrepo here: https://github.com/edestecd/puppet-software/blob/master/.fixtures.yml

Any modules that are needed during test need to be in there, so it can pull them down for the tests.

edestecd commented 6 years ago

More scenarios... I like it!

bittner commented 6 years ago

The existence test fails on a file that comes with the repository cloned by vcsrepo. The file does exist on my system, so it looks like it is actually installed.

Can you explain why the test is failing anyway? Does cloning the repo fail on the CI server?

edestecd commented 6 years ago

The unit tests do not actually run the puppet catalog on a real machine. That is what functional tests do. So it is just testing if that actual resource is in the catalog. Since there is no file resource declared (its actually a vcsrepo) nothing is happening.

edestecd commented 6 years ago

This https://github.com/edestecd/puppet-software/pull/10/files#diff-8fc8ca74ce336ebea64cf777b8973b3eR56 is just missing the trailing slash.

It has to exactly match the title in the manifest here: https://github.com/comsolit/puppet-software/blob/1eeff64a97d59f7924b45e135a1228f7bc11cd69/manifests/vcsscm/git.pp#L36

bittner commented 6 years ago

Yay! :tada: -- We're ready to merge!

bittner commented 6 years ago

I think you could merge this PR now. Or is anything missing?

Some Suggestions

If you want you could change the settings of the repository beforehand, and uncheck "Allow merge commits" (and optionally "Allow sqash merging") in the "Merge button" section. With "rebase merging " as a default the commit log will be clean of merge commits.

I'd also suggest to disable the unused "Wiki" and "Projects" options in the settings. But that's unrelated to merging, technically.

edestecd commented 6 years ago

Looks pretty good to me. I made one suggestion. It would be nice to get this and your other PR merged this morning, so I can cut a new version to the forge.

bittner commented 6 years ago

Looks like we're ready to push the button. :v:

bittner commented 6 years ago

A huge learning story comes to an end. Thanks for your patience! :1st_place_medal:

edestecd commented 6 years ago

Thanks for your contributions. I'll try and get a new release on the forge soon.

edestecd commented 6 years ago

@bittner 1.2.0 is on the forge.

edestecd / puppet-software

Add gitconfig and gitignore parameters (Git) #10

Envisioned Concept

Edge Cases

Biased But Useful

Some Suggestions