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

Fix repo refresh #199

Merged
merged 9 commits into from
Jun 14, 2016
Merged

Fix repo refresh #199

merged 9 commits into from
Jun 14, 2016

Conversation

tdm4
Copy link
Contributor

@tdm4 tdm4 commented Jun 14, 2016

Hi, I found an issue where it takes 2 runs of puppet to fix the package installation. Now the packages correctly require Class[::apt::update] so they install correctly the first time.

@tdm4
Copy link
Contributor Author

tdm4 commented Jun 14, 2016

@mmoll I'm not sure why it's showing my older commits from #190 .. but as you can see, only 3 files changed in this PR. Do you know how I'd fix the commits? Do I have to squash them again?

@mmoll
Copy link
Contributor

mmoll commented Jun 14, 2016

I guess you did branch off from a branch that did hold the old history from before your commits were squashed when merging #190. In general I recommend to always have master/develop of a fork in sync with the upstrema repo, but never do commits to it, then use topic branches for PRs to the upstream repo and when opening new PRs, re-sync master/develop from upstream and branch off a new topic branch from that. Anyway, from a mayflower/puppet-php perspective it's perfectly fine to squash all these commits together into one when merging.

@@ -27,7 +27,12 @@
}

if $::operatingsystem == 'Ubuntu' {
ensure_packages(["${php::globals::package_prefix}xml"])
if ( ! defined(Package["${php::globals::package_prefix}xml"])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a way around that? using defined would really be a last resort measurement

Copy link
Contributor

Choose a reason for hiding this comment

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

using ensure_packages from stdlib might be in order here.

@tdm4
Copy link
Contributor Author

tdm4 commented Jun 14, 2016

@mmoll Hmm.. I tried squashing these to one commit but I am getting an error: Cannot 'squash' without a previous commit
Any ideas? The command I ran was: git rebase -i HEAD~7 since my fork is 7 commits ahead.

@tdm4
Copy link
Contributor Author

tdm4 commented Jun 14, 2016

I wonder if I'm better off re-forking mayflower-php master and redoing this PR? I can't seem to squash the commits. I suppose I can leave it for now.

@tdm4
Copy link
Contributor Author

tdm4 commented Jun 14, 2016

@mmoll I think ensure_packages might take a hash of resources. Going to see if I can clean this up.

@mmoll
Copy link
Contributor

mmoll commented Jun 14, 2016

you can always grab the current mayflower master and force push it into the master branch of your fork and then cherry-pick your commit into another branch and force push into the fix_repo_refresh of your fork.

@tdm4
Copy link
Contributor Author

tdm4 commented Jun 14, 2016

OK I've sorted the ugly if ( ! defined(Package[...])) stuff. Re-checking stdlib, it does take resources in a hash after the package list which is ace. So I've amended the PR to suit.

@mmoll mmoll merged commit 7967a64 into voxpupuli:master Jun 14, 2016
@mmoll
Copy link
Contributor

mmoll commented Jun 14, 2016

merged, thanks @tdm4!

@tdm4 tdm4 deleted the fix_repo_refresh branch June 14, 2016 13:44
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.

2 participants