Integration Candidate 20200212

[astrogeco]

Describe the contribution
Fix #210, #308, and #489

Testing performed
Steps taken to test the contribution:

Current CI passes
Test 4 in Enhanced CI in https://github.com/nasa/cFS/pull/40 passes after adding #489

Expected behavior changes

#210 - Adds a new function, CFE_SB_GetPipeIdByName, which retrieves the pipe ID given a name of a pipe.

#308 - Improvement in error reporting when using a pipe name that is already in use, or when the queue limit has been reached.

System(s) tested on

CI: Ubuntu 18.04

Additional context
N/A

Third party code
N/A

Contributor Info - All information REQUIRED for consideration of pull request
Gerardo E. Cruz-Ortiz - NASA/GSFC

[astrogeco]

CCB 20200212 - @jphickey will take a look and push to the ic

[jphickey]

Took a look at this -- it looks like a unit test case in sb_UT.c is testing for CFE_SB_UNSUBSCRIPTION_RPT_EID but this event definition is not in the cfe_sb_events.h file nor does the FSW reference this either. It looks like the sb_UT.c file might be including a merge of something that the FSW does not include.

Short answer -- this IC is definitely broken and needs more investigation to see where this came from.

[jphickey]

I tracked down the issue with this IC to a incorrect merge conflict resolution between #210 and #308.

The unit test function Test_Unsubscribe_SubscriptionReporting should have been entirely removed, as one of these changes removed the function but the other changed the event count causing a conflict. Should be resolved properly in 6c4b748 and the UT passes for me and it also appears to pass for Travis now.

As a side note, trying to track down what happened here between all the different pull requests was enough to make your head spin. If its at all possible, we should try to streamline the merge process a bit, as I see a whole lot of branch-jockeying in what basically amounts to two separate pull requests and two separate merges for each individual change.

[astrogeco]

I tracked down the issue with this IC to a incorrect merge conflict resolution between #210 and #308.

The unit test function Test_Unsubscribe_SubscriptionReporting should have been entirely removed, as one of these changes removed the function but the other changed the event count causing a conflict. Should be resolved properly in 6c4b748 and the UT passes for me and it also appears to pass for Travis now.

As a side note, trying to track down what happened here between all the different pull requests was enough to make your head spin. If its at all possible, we should try to streamline the merge process a bit, as I see a whole lot of branch-jockeying in what basically amounts to two separate pull requests and two separate merges for each individual change.

Thanks Joe! I used the web interface for the merges. The challenge was knowing which one to merge first and trying to avoid a merge into the feature branches and not wanting to force a rebase onto the feature branches.

Also, what do you mean by branch-jockeying?

[jphickey]

I just mean changes going from branch to branch. In this example the ic seemed to get merged back into the original pull request branch (Chris's branch) as a normal merge - reverse from the normal flow - and then subsequently merged back into the IC as a rebase (maybe? I'm guessing). I got really confused trying to figure out what happened, but some odd back-and-forth seemed to happen here.

In the previous system (prior to our github transition) where I was handling the merges I also would do the final "ic" merge to the mainline branch as a fast-forward, so as to avoid adding another superfluous merge commit which clouds the history (we should have recorded the CCB review actions in the merge to the IC branch, so it didn't seem necessary to also record a separate merge from the IC to mainline). We should reconsider this option.

And I'm not a fan of using a web interface for merges... command line all the way. Particularly if there is anything requiring a conflict resolution. Most importantly that gives an opportunity to verify/test it locally before pushing a (possibly) broken merge.