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

Refactoring #29

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

Refactoring #29

wants to merge 43 commits into from

Conversation

disaster37
Copy link

Pull Request (PR) description

  1. Convert project to PDK
  2. Use new Puppet syntaxe
  3. Rewrite all code base on https://github.com/pcfens/puppet-filebeat
  4. Add support of winlogbeat 6
    5 Add the ability to download winlogbeat from other source than http:// like puppet://

This Pull Request (PR) fixes the following issues

Fixes #1
Fixes #2
Fixes #10

@vox-pupuli-tasks
Copy link

Dear @disaster37, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

2 similar comments
@vox-pupuli-tasks
Copy link

Dear @disaster37, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@vox-pupuli-tasks
Copy link

Dear @disaster37, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@vox-pupuli-tasks
Copy link

Dear @disaster37, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@vox-pupuli-tasks
Copy link

Dear @disaster37, thanks for the PR!

This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@ralfbosz
Copy link

ralfbosz commented Mar 4, 2020

Would love to see this fixed/merged, looking for the option to add 'processors' which is not in the v1.0.0 release...

@jacobmw
Copy link

jacobmw commented Apr 14, 2020

Any reason not to merge at this time? Would be nice to be able to use this version of the module to manage winlogbeat version >= 6

@robrankin
Copy link

@bastelfreak Any chance this, or something similar can be merged? Not having processor support in the module is very painful.

@bastelfreak
Copy link
Member

@robrankin we cannot merge this because of the merge conflicts :(
if someone resolves them and submits it as a new PR, I'm happy to review it

@marcinbojko
Copy link

marcinbojko commented Jul 20, 2020

@robrankin we cannot merge this because of the merge conflicts :(
if someone resolves them and submits it as a new PR, I'm happy to review it

I think merge conflicts are mostly from moving to vox populi part.

@jacobmw
Copy link

jacobmw commented Jan 7, 2021

@disaster37 are you able to resolve these conflicts? If you need help I am willing to do so though not sure how to go about it in terms of where to make the changes. Perhaps if I need write access to your repo? Or do I fork yours and make a PR to that?

@kenyon
Copy link
Member

kenyon commented Jan 8, 2021

@disaster37 are you able to resolve these conflicts? If you need help I am willing to do so though not sure how to go about it in terms of where to make the changes. Perhaps if I need write access to your repo? Or do I fork yours and make a PR to that?

I'd suggest creating a new PR based on the changes in this one, or redoing the changes of this one, based on the current master branch.

@disaster37
Copy link
Author

Hi will try to look that the next week

@bastelfreak
Copy link
Member

@disaster37 the history still looks a bit strange and needs to be rebased. If you need help with that you can join our IRC channel #voxpupuli on freenode or our slack channel #voxpupuli on http://slack.puppet.com/

@disaster37
Copy link
Author

I think is now good ?

@kenyon
Copy link
Member

kenyon commented Jan 12, 2021

I think is now good ?

IMO these changes should have been split into multiple pull requests, this one is just too big. Also, separately, you should clean up the commit history so that it only has commits which make correct changes (i.e., no "fix my previous mistake" and no "try this thing and see if it works" commits), and the commits are each logically standalone changes. There are many resources on commit history, such as this. We could squash this all down to a single commit, but again, it looks like there are changes about separate topics that really should be separate commits, in order to have an comprehensible change history.

@kenyon
Copy link
Member

kenyon commented Jan 12, 2021

Another reason this should be separate PRs is that PR titles are used in the changelog, and a change like "refactoring" tells the reader nothing about what was changed.

Signed-off-by: disaster37 <linuxworkgroup@hotmail.com>
Signed-off-by: disaster37 <linuxworkgroup@hotmail.com>
Signed-off-by: disaster37 <linuxworkgroup@hotmail.com>
Signed-off-by: disaster37 <linuxworkgroup@hotmail.com>
Signed-off-by: disaster37 <linuxworkgroup@hotmail.com>
Signed-off-by: disaster37 <linuxworkgroup@hotmail.com>
Signed-off-by: disaster37 <linuxworkgroup@hotmail.com>
Signed-off-by: disaster37 <linuxworkgroup@hotmail.com>
@jstraw
Copy link

jstraw commented Jan 28, 2022

This really looks like it could use someone to cherry pick the actual changes to the manifests/templates without the PDK changes (voxpupuli has its own modulesync/test harness) into a separate PR based off master and create a new PR based on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants