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

Manage git remote for device groups #30

Merged
merged 1 commit into from
Aug 25, 2021

Conversation

nhemingway
Copy link
Contributor

Closes #27

}
}

it {
Copy link
Collaborator

Choose a reason for hiding this comment

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

also should test the two file resources added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The existing test asserts that we do in fact behave correctly if asked to manage remote urls (though I should probably add a negative test).

That being the case, whilst I'm happy to add the tests you request, what benefit would they add over the existing test?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Besides the exec, which you test for, you also added two file resources "post-commit hook for ${name}" and "rancid default git remote ${name}" which should be tested in that same block and are not being tested elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that all three resource are enclosed in the same conditional block, there is no way that any of the three can be included in the catalogue without all of them being included. Therefore, there is no benefit including tests for all three, because they will either pass or fail together. I included a test for the exec purely to test that the condition is working correctly. If we add tests for the other two resources, then surely we're simply repeating the manifest, but not really adding any value?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You do not need to include all of the attributes though there should be basic, this resource should exist. If the coverage bits were enabled you would get a report at the end showing untested resources. While this might seem like extra work now, as the code changes over time, this ensures proper testing.

 it { should contain_file('asdf') }

@ghoneycutt
Copy link
Collaborator

@nhemingway I see you are giving this module some needed attention :) You might look to update the metadata.json to support newer puppet versions and the .travis.yml to test them. Ideally this would support Puppet 6 and 7.

@nhemingway
Copy link
Contributor Author

@nhemingway I see you are giving this module some needed attention :) You might look to update the metadata.json to support newer puppet versions and the .travis.yml to test them. Ideally this would support Puppet 6 and 7.

I've spawned that work out to #31 as I think it's more involved than just updating a couple of config files. (Not that I know how to update them having never worked with travis before)

@ghoneycutt ghoneycutt merged commit 2fb6cf1 into eoly:master Aug 25, 2021
@nhemingway nhemingway deleted the nh/27-git-remote branch August 27, 2021 15:04
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.

Add ability to setup git remote repo
2 participants