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

iox-#1919-fix-max-group-name-length #1925

Conversation

atirehgram
Copy link
Contributor

@atirehgram atirehgram commented Feb 27, 2023

Pre-Review Checklist for the PR Author

  1. Code follows the coding style of CONTRIBUTING.md
  2. Tests follow the best practice for testing
  3. Changelog updated in the unreleased section including API breaking changes
  4. Branch follows the naming format (iox-123-this-is-a-branch)
  5. Commits messages are according to this guideline
  6. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  7. Relevant issues are linked
  8. Add sensible notes for the reviewer
  9. All checks have passed (except task-list-completed)
  10. All touched (C/C++) source code files from iceoryx_hoofs are added to ./clang-tidy-diff-scans.txt
  11. Assign PR to reviewer

Notes for Reviewer

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Code according to our coding style and naming conventions
  • Unit tests have been written for new behavior
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

@elfenpiff
Copy link
Contributor

@atirehgram did you sign the eclipse eca yet? https://www.eclipse.org/legal/ECA.php

This is required by every committer. For this you require an eclipse account which uses the same email address you used for the commits here and has signed the eclipse committer agreement.

Copy link
Contributor

@elfenpiff elfenpiff left a comment

Choose a reason for hiding this comment

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

Looks good, as soon as the ECA is signed it can be merged.

If you like you can add your name in the copyright header of every file you have modified:

// Copyright (c) 2023 by ???. All rights reserved.

But this is optional and up to you.

@atirehgram atirehgram force-pushed the iox-1919-fix-max-group-name-length branch from eedd03a to e499b72 Compare February 27, 2023 16:02
@codecov
Copy link

codecov bot commented Feb 27, 2023

Codecov Report

Merging #1925 (e499b72) into master (adbed1b) will increase coverage by 0.03%.
The diff coverage is n/a.

❗ Current head e499b72 differs from pull request most recent head c573c94. Consider uploading reports for the commit c573c94 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1925      +/-   ##
==========================================
+ Coverage   75.43%   75.46%   +0.03%     
==========================================
  Files         384      384              
  Lines       15206    15199       -7     
  Branches     2148     2148              
==========================================
  Hits        11470    11470              
+ Misses       3062     3058       -4     
+ Partials      674      671       -3     
Flag Coverage Δ
unittests 75.12% <ø> (+0.03%) ⬆️
unittests_timing 15.47% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...oofs/source/log/building_blocks/console_logger.cpp 80.59% <0.00%> (-0.29%) ⬇️
...unctional/include/iox/detail/storable_function.inl 95.00% <0.00%> (-0.09%) ⬇️
iceoryx_hoofs/vocabulary/include/iox/variant.hpp 100.00% <0.00%> (ø)
tools/introspection/source/introspection_app.cpp 0.00% <0.00%> (ø)
...oryx_hoofs/container/include/iox/detail/vector.inl 98.68% <0.00%> (ø)
...ryx_hoofs/vocabulary/include/iox/detail/string.inl 98.81% <0.00%> (ø)
..._hoofs/memory/include/iox/detail/scoped_static.inl 0.00% <0.00%> (ø)
...hoofs/filesystem/include/iox/detail/filesystem.inl 98.88% <0.00%> (ø)
...ofs/memory/include/iox/detail/relative_pointer.inl 96.59% <0.00%> (ø)
...design/include/iox/detail/functional_interface.inl 95.12% <0.00%> (ø)
... and 5 more

@atirehgram atirehgram force-pushed the iox-1919-fix-max-group-name-length branch from e499b72 to b2b0047 Compare February 28, 2023 07:33
@atirehgram
Copy link
Contributor Author

@atirehgram did you sign the eclipse eca yet? https://www.eclipse.org/legal/ECA.php

This is required by every committer. For this you require an eclipse account which uses the same email address you used for the commits here and has signed the eclipse committer agreement.

Sorry, I forgot with which email I had signed it. It should be fine now, could you please retrigger the check?

@elfenpiff
Copy link
Contributor

@atirehgram eclipse does not want to verify it. It states: margherita.protti@****.com failed. Could you may please check again if you signed with the same email address the ECA you committed with.

And I will retry it in the afternoon again - maybe eclipse requires just some more time. Most of the times eclipse is just a little bit weird and slow.

@mossmaurice mossmaurice added the bugfix Solves a bug label Feb 28, 2023
@atirehgram
Copy link
Contributor Author

I double checked the mail in the ECA and I also retriggered the check manually twice, but it keeps failing.

@elfenpiff
Copy link
Contributor

@atirehgram we have a hard internal deadline for a feature until Thursday. I would retrigger the check again and on Friday I would start digging here deeper.

But when we are lucky the problem is just vanished until then.

@elfenpiff
Copy link
Contributor

@atirehgram I think eclipse somehow screws up here and after trying the 1000th captcha challenge of trying to find a hamster in its natural environment I would propose the following.

  • Could you please split up your commits into two with the correct author and force push it. Maybe the eclipse thingy has your email cached somewhere and therefore it does not validate it.
  • If this does not work I would create an eclipse bug ticket.

One question, did you change the commits author or had multiple authors for one commit? This could maybe explain the eclipse bug - but it is still an eclipse bug.

@elfenpiff
Copy link
Contributor

@atirehgram I have found the problem. When you take a look at your commit with git log --format=fuller you see:

commit b2b0047e7516e7e0e3e38f5493111255f20f724a (HEAD -> iox-1919-fix-max-group-name-length, foo/iox-1919-fix-max-group-name-length)
Author:     Margherita Protti <[email protected]>
AuthorDate: Mon Feb 27 14:17:33 2023 +0100
Commit:     Margherita Protti <[email protected]>
CommitDate: Tue Feb 28 08:32:27 2023 +0100

So it seems you have somehow an author but a different committer (I didn't know that this was possible). The e-mail in the author section has to sign the ECA. And usually the author and committer are the same I suspect. So could you please use the same email for commit and author and then we can get this PR in.

I think you may have to rebase to master again since it shows now a merge conflict with our release notes.

But when we handled this PR sucessfully the next PRs from your side will be much smoother. Eclipse with its ECA is not so welcoming to newcomers :/ ...

@atirehgram atirehgram force-pushed the iox-1919-fix-max-group-name-length branch from b2b0047 to 0a64113 Compare March 6, 2023 13:57
Copy link
Contributor

@FerdinandSpitzschnueffler FerdinandSpitzschnueffler left a comment

Choose a reason for hiding this comment

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

@atirehgram Thanks for your contribution and your patience!

@FerdinandSpitzschnueffler FerdinandSpitzschnueffler merged commit 0b8052e into eclipse-iceoryx:master Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Solves a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MAX_GROUP_NAME_LENGTH should be at least equal to MAX_USER_NAME_LENGTH
4 participants