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

[Filebeat] Fix of syslog parsing for Priority value <0> ( #11010 ) #11011

Closed
wants to merge 1 commit into from
Closed

[Filebeat] Fix of syslog parsing for Priority value <0> ( #11010 ) #11011

wants to merge 1 commit into from

Conversation

DebashisMondal
Copy link
Contributor

Fix of #11010 in elastic/beat branch

Fix of #11010 in elastic/beat branch
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@kvch
Copy link
Contributor

kvch commented Mar 1, 2019

@kvch kvch requested a review from ph March 1, 2019 09:49
@kvch
Copy link
Contributor

kvch commented Mar 1, 2019

@ph could you please review this fix?

@DebashisMondal
Copy link
Contributor Author

I think this part also needs to be fixed: https://github.com/elastic/beats/blob/master/filebeat/input/syslog/syslog_rfc3164.rl#L10

I don't think any changes is required in this file.

I have locally verified the fix which is perfectly working. If any case the above need to be change then kindly let me know.

@kvch kvch added review needs_backport PR is waiting to be backported to other branches. Filebeat Filebeat labels Mar 1, 2019
@kvch
Copy link
Contributor

kvch commented Mar 1, 2019

In the meantime could you please "sign" the CLA?

@kvch kvch added the needs CLA User must sign the Elastic Contributor License before review. label Mar 1, 2019
@DebashisMondal
Copy link
Contributor Author

DebashisMondal commented Mar 1, 2019

In the meantime could you please "sign" the CLA?

I am able to sign this on Monday. Is it fine?
As I am contributing for the first time so I want to ask you @kvch that this CLA agreement is for every contribution or only for this fix.

@ph
Copy link
Contributor

ph commented Mar 1, 2019

Changes look good to me.

@DebashisMondal you only need to sign the CLA once and it will be valid for all the other PRs you are making.

Can you also add an entry to the changelog?

@DebashisMondal
Copy link
Contributor Author

@ph can I change the agreement type in future?
Yes, I will make an entry in changelog.

@kvch
Copy link
Contributor

kvch commented Mar 4, 2019

jenkins test this

@ph
Copy link
Contributor

ph commented Mar 4, 2019

@DebashisMondal changing from what to what?

@DebashisMondal
Copy link
Contributor Author

@ph in which changelog I have to change? In master or in 6.6 .
In which changelog file I have to change

  1. CHANGELOG-developer.asciidoc
  2. CHANGELOG-developer.next.asciidoc
  3. CHANGELOG.asciidoc
  4. CHANGELOG.next.asciidoc

Kindly let me know as I am contributing for the first time.

@kvch
Copy link
Contributor

kvch commented Mar 5, 2019

Please add the entry to CHANGELOG.next.asciidoc on master.

@DebashisMondal DebashisMondal changed the title [Filebeat] Fix of syslog parsing for Priority value ( #11010 ) [Filebeat] Fix of syslog parsing for Priority value <0> ( #11010 ) Mar 6, 2019
@DebashisMondal
Copy link
Contributor Author

@kvch can you please verify the pull request for changelog? Why it gets failed during Integration. Can you please let me know as I am new to this.

Thanks

@kvch
Copy link
Contributor

kvch commented Mar 6, 2019

Those issues are not related to your change. Do you mind adding the new changelog entry to this pull request?

@DebashisMondal
Copy link
Contributor Author

@kvch, Actually I am not much more familiar with the pulls.
Basically in this pull request I have done the changes in 6.6 version but you suggested me to add the changelog in master, so for that reason I have made the changes with new pull request but somehow it fails.

If you don't mind can you add the changelog in behalf of me. If you have any suggestion kindly let me know.

@kvch
Copy link
Contributor

kvch commented Mar 6, 2019

Hm. That's interesting. I recall checking the base branch and it said master. I might dreamed that. Sorry about that.
Could you please open a new pull request to master which contains both this fix and the changelog entry? Our strategy is to open a PR to master and then backport the changes to the required versions.

@DebashisMondal
Copy link
Contributor Author

DebashisMondal commented Mar 6, 2019

@kvch Is it not possible to merge the changes from this pull request to the master. Is it mandatory to open pull request in master.

I have also observed that other contributor changes in branch 6.6 and it is merged to master. Is not it possible for this case?

In addition before opening pull request I want to know why the chagelog regarding PR which I have mentioned here gets failed in Build. Can you tell me the reasons?

And one more thing in Changelog of master I observed that the pull request for the fix is referred in other cases. But if I open a new pull request in master then what will the reference for the fix (Bug reference or anything else?

@DebashisMondal
Copy link
Contributor Author

DebashisMondal commented Mar 6, 2019

@ph As you mentioned that changes looks good to you, so kindly approve the changes.

@kvch
Copy link
Contributor

kvch commented Mar 6, 2019

You could change the base branch to master of this PR instead of opening a new one.

You changelog PR is failing because Travis failed to install the requiested dependencies of an unrelated job: https://travis-ci.org/elastic/beats/jobs/502463779#L420
It's nothing to worry about.

@DebashisMondal DebashisMondal changed the base branch from 6.6 to master March 6, 2019 11:56
@DebashisMondal DebashisMondal changed the base branch from master to 6.6 March 6, 2019 11:59
@DebashisMondal
Copy link
Contributor Author

@kvch when I am changing the base branch to master it is showing some conflicts, so what to do now?

@DebashisMondal DebashisMondal changed the base branch from 6.6 to master March 6, 2019 12:31
@kvch
Copy link
Contributor

kvch commented Mar 6, 2019

You could rebase it. But I think it would be easier to apply the patch to master and force push this branch with that single commit.

@DebashisMondal DebashisMondal changed the base branch from master to 6.6 March 6, 2019 12:40
@DebashisMondal
Copy link
Contributor Author

@kvch In addition I want to know that when the review will be marked as completed by @ph. Is there any gap?

@ph ph changed the base branch from 6.6 to master March 8, 2019 15:40
@ph ph changed the base branch from master to 6.6 March 8, 2019 15:40
@ph
Copy link
Contributor

ph commented Mar 8, 2019

I am approving the change for the file, but could you target the master branch and rebase your PR on top of master and include #11107 as part of the same PR this make it easier to backport everything and make a cleaner history. If you have problem doing that, myself or @kvch could do it but we would have to do some trickery because we want to keep your authorship of the work.

@DebashisMondal
Copy link
Contributor Author

DebashisMondal commented Mar 13, 2019

@ph, Basically I committed the fix in version 6.6 but as suggested by @kvch when I am trying to rebase it to master branch I am getting some conflicts. I have changed the changelog in master branch #11107 .

Keeping my authorship, you (@ph and @kvch ) can do whatever is required.

And one more thing @ph, as you approved the changes kindly change the review status, it is still showing "Awaiting requested review from ph"

@kvch
Copy link
Contributor

kvch commented Mar 13, 2019

To eliminate the conflict, you could try to cherry-pick your commit on top of master.

Update your master branch. Make sure you don't have any unstaged changes, as resetting removes everything. You can skip this step, if your master branch is up to date with the master in this repo (upstream is this repo).

git fetch upstream
git checkout master
git reset --hard upstream/master

Cherry-pick this commit on top of a new branch.

git checkout -b fix-filebeat-syslog-priority
git cherry-pick 2b2794debb314bfe73d8ccd77190c2dc0e4bc5fc
git push origin fix-filebeat-syslog-priority

After you have pushed the branch, open a PR with the base branch master. Your new PR will be approved if it's opened for the correct base branch.

Let me know if you need further assistance.

@DebashisMondal
Copy link
Contributor Author

@kvch can I also cherry-pick the commit of #11107 ?

@kvch
Copy link
Contributor

kvch commented Mar 14, 2019

Sure. That would be even better. :)

@DebashisMondal
Copy link
Contributor Author

Actually I want to know @kvch that after opening new PR this PR will be valid or not?

@DebashisMondal
Copy link
Contributor Author

DebashisMondal commented Mar 14, 2019

@kvch I am getting below error during cherry-pick

[root@localhost beats] git cherry-pick 2b2794debb314bfe73d8ccd77190c2dc0e4bc5fc
fatal: bad object 2b2794debb314bfe73d8ccd77190c2dc0e4bc5fc

I am little bit confused that I have to create a fork of master branch then I have to follow these steps or Directly cloning master branch I have to do all the steps.

When I am doing after forking the master branch then everything is working but when I am trying to do this things directly with cloning master branch then above error message is displaying

@DebashisMondal
Copy link
Contributor Author

DebashisMondal commented Mar 18, 2019

@kvch and @ph I have created a new pull request for master branch kindly look at this #11288

@DebashisMondal
Copy link
Contributor Author

@kvch as #11288 is approved, So can I close this pull request?

@kvch
Copy link
Contributor

kvch commented Mar 19, 2019

Closing as #11288 has been opened.

@kvch kvch closed this Mar 19, 2019
@andrewkroh andrewkroh removed the needs_backport PR is waiting to be backported to other branches. label Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Filebeat Filebeat needs CLA User must sign the Elastic Contributor License before review. review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants