Skip to content

Refactoring - #29

Open
disaster37 wants to merge 43 commits into
voxpupuli:masterfrom
disaster37:refactoring
Open

Refactoring#29
disaster37 wants to merge 43 commits into
voxpupuli:masterfrom
disaster37:refactoring

Conversation

@disaster37

Copy link
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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

ralfbosz commented Mar 4, 2020

Copy link
Copy Markdown

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

@jacobmw

jacobmw commented Apr 14, 2020

Copy link
Copy Markdown

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
Copy Markdown

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

@bastelfreak

Copy link
Copy Markdown
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

marcinbojko commented Jul 20, 2020

Copy link
Copy Markdown

@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

jacobmw commented Jan 7, 2021

Copy link
Copy Markdown

@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

kenyon commented Jan 8, 2021

Copy link
Copy Markdown
Member

@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
Copy Markdown
Author

Hi will try to look that the next week

@bastelfreak

Copy link
Copy Markdown
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
Copy Markdown
Author

I think is now good ?

@kenyon

kenyon commented Jan 12, 2021

Copy link
Copy Markdown
Member

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

kenyon commented Jan 12, 2021

Copy link
Copy Markdown
Member

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

jstraw commented Jan 28, 2022

Copy link
Copy Markdown
Contributor

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

Development

Successfully merging this pull request may close these issues.

queue_size has been removed from winlogbeat bug: manifests/install.pp line 27 and 39 Puppet inserting --- before outputs in yml

10 participants