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

reset the counter after processed commits in branch #1058

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shanchenqi
Copy link
Contributor

Signed-off-by: Chenqi Shan [email protected]

@coveralls
Copy link

coveralls commented May 25, 2022

Pull Request Test Coverage Report for Build 2403637097

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 10 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.006%) to 82.335%

Files with Coverage Reduction New Missed Lines %
/home/runner/work/grimoirelab-elk/grimoirelab-elk/grimoire_elk/enriched/git.py 10 71.58%
Totals Coverage Status
Change from base Build 2383854769: -0.006%
Covered Lines: 8828
Relevant Lines: 10722

💛 - Coveralls

@sduenas
Copy link
Member

sduenas commented May 25, 2022

@shanchenqi I'm not familiar with this part of the code, what is it for?

@shanchenqi
Copy link
Contributor Author

@shanchenqi I'm not familiar with this part of the code, what is it for?

Hi, @sduenas , Let me explain the bugs and my code. During the study enrich_git_branches , we reset the counter only after if commit_count == MAX_BULK_UPDATE_SIZE:. We should also reset the counter after if commit_count: by the same way. Otherwise, to_process list will be added to next for Loop when branched has been added by "if commit_count:".

@jjmerchante
Copy link
Contributor

The problem here is that some commits from one branch, for example from development, will also be tagged with another branch name like master. The enriched items will be:

{
    ...
    branch: ["development", "master"]
}

while it should be:

{
    ...
    branch: ["development"]
}

This only happens when the enrich_git_branches study is enabled.

Merging this PR will fix the problem, but items enriched with the wrong branch won't be updated.

@jjmerchante
Copy link
Contributor

@shanchenqi The PR looks ok but can we improve the commit message? We usually follow some guidelines: https://github.com/chaoss/grimoirelab/blob/master/CONTRIBUTING.md#guidelines-to-follow-to-write-good-commit-messages

We are also starting to include a changelog file with information about the PR for the release notes. You might need to install the package release-tools and run changelog and follow the instructions. We can do that step for you.

@shanchenqi
Copy link
Contributor Author

@shanchenqi The PR looks ok but can we improve the commit message? We usually follow some guidelines: https://github.com/chaoss/grimoirelab/blob/master/CONTRIBUTING.md#guidelines-to-follow-to-write-good-commit-messages

We are also starting to include a changelog file with information about the PR for the release notes. You might need to install the package release-tools and run changelog and follow the instructions. We can do that step for you.

OK, @jjmerchante thanks for your help! I will follow the instructions in my next PR.

@sduenas
Copy link
Member

sduenas commented May 26, 2022

Btw @shanchenqi , you don't need to create a new PR, you can just update this one ammending the commits and forcing an update of the branch.

@vchrombie
Copy link
Member

@shanchenqi you need not create a new PR, you can add the changelog & amend the commit message, and later you can force push to your branch.

The following commands should help you

$ git add <path-to-your-changelog-entry>
$ git commit --amend # update your commit message here
$ git push -f origin git-enriched

Please let us know if you need any help. Thanks for your efforts.

@shanchenqi shanchenqi force-pushed the git-enriched branch 2 times, most recently from 7fb3d20 to 2cc7bc3 Compare May 29, 2022 03:06
@shanchenqi
Copy link
Contributor Author

@shanchenqi you need not create a new PR, you can add the changelog & amend the commit message, and later you can force push to your branch.

The following commands should help you

$ git add <path-to-your-changelog-entry>
$ git commit --amend # update your commit message here
$ git push -f origin git-enriched

Please let us know if you need any help. Thanks for your efforts.

Excuse me, @vchrombie @jjmerchante there is still an error in my git commit changelog, could you help me transform it?

@shanchenqi shanchenqi force-pushed the git-enriched branch 2 times, most recently from 698ac5a to 9e0541e Compare May 29, 2022 03:22
@vchrombie
Copy link
Member

Excuse me, @vchrombie @jjmerchante there is still an error in my git commit changelog, could you help me transform it?

Hi @shanchenqi, I still don't see the changelog entry.

In case, if you are not familiar with the changelog entries, I'm adding a reference for you.
https://github.com/chaoss/grimoirelab-perceval/blob/master/CONTRIBUTING.md#changelog-entries

You can use the interactive changelog tool for this purpose which generates the changelog entry file automatically.

Please go through it and reach out in case you need any help.

@shanchenqi
Copy link
Contributor Author

shanchenqi commented May 29, 2022

@vchrombie Thank you! I added the changelog entry and all checks have passed.

@sduenas
Copy link
Member

sduenas commented Jun 3, 2022

@shanchenqi the PR looks good but I edited the changelog file and the commit message to include it. I was trying to push a commit to your branch but I don't have access to it. I can provide you the change or you can give me access to that branch.

@shanchenqi
Copy link
Contributor Author

Hi,@sduenas, thanks for your help. Maybe Git does not have branch specific permissions. Would you mind creating a pull request to my branch? After I merged it, this PR will be updated auto.

@jjmerchante
Copy link
Contributor

Hi @shanchenqi, you can update the permissions from the right side of this pull request:

image

@shanchenqi
Copy link
Contributor Author

Hi, @jjmerchante @sduenas I have already set the permissions for this pull request, could you try to push a commit to my branch again?
image

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.

5 participants