Skip to content

Improve viewer class (fully qualify ht_crypt) #88

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

DLeich
Copy link

@DLeich DLeich commented Jun 17, 2022

Pull Request (PR) description

The viewer class was failing to compile when used on puppet 7. This is because the ht_crypt function usage needed to be fully qualified.

@DLeich
Copy link
Author

DLeich commented Jul 3, 2022

Can I get a review on this? This PR replaces #58, which had to be reauthored due to a multitude of merge conflicts.

@bastelfreak
Copy link
Member

@DLeich thanks for the work. can you please rebase against our master branch so the merge commit disappears?

@DLeich
Copy link
Author

DLeich commented Aug 24, 2022

Okay this is odd. I just tested trying to rebase, and even just remove my commits for the README.md on a separate personal branch which worked, but on this branch & PR, it still adds them back, so now we have a number of extra commits which are unnecessary. Let me see if I can squash the last handful of commits to make this cleaner.

@DLeich
Copy link
Author

DLeich commented Aug 24, 2022

@bastelfreak - got the rebase situation resolved. I think this is ready now.

htpasswd: https://github.com/citrininfo/puppet-htpasswd.git
vcsrepo: https://github.com/puppetlabs/puppetlabs-vcsrepo.git
git: https://github.com/puppetlabs-toy-chest/puppetlabs-git.git
concat: https://github.com/puppetlabs/puppetlabs-concat.git

Choose a reason for hiding this comment

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

@DLeich might it be better to refer to the Forge versions, instead of the bleeding edge versions on GitHub?

Copy link
Member

Choose a reason for hiding this comment

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

no :) we refer to master/main branch for unit tests, so wen can detect breaking changes in dependencies before they are released. for acceptance tests we use the latest versions, that the metadata.json accepts, from the forge (this module currently has no acceptance tests but I will add them later

Choose a reason for hiding this comment

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

Right on! Makes sense.

I've got a module I've been struggling to add acceptance tests to; would love to see how you do that here or elsewhere.

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.

3 participants