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

use Stdlib::Fqdn type for package_keyserver / bump stdlib to 4.25 #759

Merged
merged 2 commits into from Mar 17, 2018

Conversation

marcdeop
Copy link
Contributor

@marcdeop marcdeop commented Mar 6, 2018

As discussed #746 here is a PR to fix the data type for package_keyserver.

This is cannot be merged until there is a release of puppetlabs-stdlib that include the proper datatype (it's only on master right now)

@bastelfreak
Copy link
Member

I restarted the travis jobs. @marcdeop https://forge.puppet.com/puppetlabs/stdlib got released 3 days ago. Can you please bump the required version in metadata.json?

@bastelfreak bastelfreak added the needs-work not ready to merge just yet label Mar 16, 2018
@marcdeop marcdeop force-pushed the improvePackageKeyServerValidator branch from aff071d to c0c5ba9 Compare March 16, 2018 22:55
@marcdeop
Copy link
Contributor Author

Commit 7ae6079 bumps minimum stdlib version both in metadata.json and in .fixtures.yml so we can make sure that tests run the same way as the production code :-)

.fixtures.yml Outdated
@@ -4,6 +4,7 @@ fixtures:
repo: 'git://github.com/puppetlabs/puppetlabs-apt'
stdlib:
repo: 'git://github.com/puppetlabs/puppetlabs-stdlib'
ref: "4.25.0"
Copy link
Member

Choose a reason for hiding this comment

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

please don't bump it here. We always want to test against the master branch to catch unreleased errors early.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it from the commit even though I don't understand the reason.
Which unreleased errors? From the stdlib library? That would not make much sense...

I can only see downsides on doing this: you are testing with code that it will be different from what the actual production code will be based on. How does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

that's a complicated story. In the past our modules broke often because a breaking change was introduced into a dependency, back then we references releases in the .fixtures.yml. We pinned it to the lowest known working version. The puppet module tool always tries to download the latest version for dependencies.

tldr: We noticed that dependencies aren't compatible in the latest version only because it failed for our users and they reported it.

What we now do is a bit smarter. We always test against the master branches of our dependencies. So we notice during our regular PRs to our modules, if a dependency isn't working anymore. This is all related to our unit tests.


Our acceptance tests are different. We start docker instances on travis, install dependencies from our metadata.json, execute puppet code and check if everything is working. We're simulating our users.

Combination of unit and acceptance tests are, in my opinion, the best way to address/track breaking changes.

This was a rather long explanation and I'm not a native speaker. If this text raises more questions than it answers please feel free to join our IRC channel #voxpupuli on freenode and we can talk about this.

@marcdeop marcdeop force-pushed the improvePackageKeyServerValidator branch from 7ae6079 to de0a96e Compare March 16, 2018 23:12
@juniorsysadmin juniorsysadmin added enhancement New feature or request backwards-incompatible and removed needs-work not ready to merge just yet labels Mar 17, 2018
@bastelfreak bastelfreak merged commit cf83637 into voxpupuli:master Mar 17, 2018
@bastelfreak bastelfreak changed the title feat: use Stdlib::Fqdn type for package_keyserver use Stdlib::Fqdn type for package_keyserver / bump stdlib to 4.25 Mar 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants