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

Fast-RTPS external dependency Quality Declaration #357

Closed
wants to merge 15 commits into from

Conversation

Blast545
Copy link
Contributor

This is a quality declaration for eProsima Fast-RTPS external dependency for ROS2 packages as described in the proposed REP-2004.

This declaration represents some thoughts regarding the current state of this external dependency and how this affect Quality Level 1 for ROS2 packages. Open to discussion, any thoughts are welcome.

Signed-off-by: Jorge Perez jjperez@ekumenlabs.com

@Blast545 Blast545 self-assigned this Mar 30, 2020
@Blast545 Blast545 changed the title FastRtps external dependency Quality Declaration Fast-RTPS external dependency Quality Declaration Mar 30, 2020
Copy link
Contributor

@brawner brawner left a comment

Choose a reason for hiding this comment

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

It looks like this needs to be updated for our most current style.

Quality_Declaration_fastrtps.md Outdated Show resolved Hide resolved
Quality_Declaration_fastrtps.md Outdated Show resolved Hide resolved
Quality_Declaration_fastrtps.md Show resolved Hide resolved
Quality_Declaration_fastrtps.md Show resolved Hide resolved
Quality_Declaration_fastrtps.md Show resolved Hide resolved
Quality_Declaration_fastrtps.md Outdated Show resolved Hide resolved
Quality_Declaration_fastrtps.md Outdated Show resolved Hide resolved
Quality_Declaration_fastrtps.md Outdated Show resolved Hide resolved
Quality_Declaration_fastrtps.md Outdated Show resolved Hide resolved
Quality_Declaration_fastrtps.md Show resolved Hide resolved
Quality_Declaration_fastrtps.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

please review typos and links

Quality_Declaration_fastrtps.md Outdated Show resolved Hide resolved
Quality_Declaration_fastrtps.md Show resolved Hide resolved
Quality_Declaration_fastrtps.md Outdated Show resolved Hide resolved
Quality_Declaration_fastrtps.md Outdated Show resolved Hide resolved
Quality_Declaration_fastrtps.md Outdated Show resolved Hide resolved
Quality_Declaration_fastrtps.md Show resolved Hide resolved
Blast545 and others added 11 commits May 1, 2020 10:44
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Co-authored-by: Alejandro Hernández Cordero <alejandro@openrobotics.org>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Co-authored-by: Alejandro Hernández Cordero <alejandro@openrobotics.org>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@Blast545 Blast545 requested a review from ahcorde May 1, 2020 14:12
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@JaimeMartin
Copy link
Contributor

Please do not close this PR yet. In a quick review, I see there is missing information, for example, we do have public coverage reports.

@MiguelCompany , could you help on this one?

@Blast545
Copy link
Contributor Author

Blast545 commented May 4, 2020

@JaimeMartin Sure! Let me know if there is anything missing before merging, I opened a similar PR in #360 that maybe you'd like to give a check as well.

Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@Blast545
Copy link
Contributor Author

Blast545 commented May 7, 2020

@JaimeMartin @MiguelCompany are you checking on this? Please let me know, I'd like to get this merged without missing information.

@MiguelCompany
Copy link
Collaborator

MiguelCompany commented May 8, 2020

@Blast545 Yeah, I have some comments on this.

We are now enforcing DCO on the PRs. You can see it, for instance, on PR 1190. We still have to add this to our contibuting.md, but we will have it before 2.0.0 is released.

Regarding coverage, our nightlies do have coverage reports, see an example here

Even though we have committed mistakes in the past, we do try to follow Semantic Versioning. We have encouraged all the team to be specially careful on the reviews about changes on public headers, and will not allow mistakes from 2.0.0 onwards. API breaks should only be allowed between major version changes, if an ABI break is required, it should be done between minor versions and be clearly stated on the release notes.

@ahcorde
Copy link
Contributor

ahcorde commented May 8, 2020

@MiguelCompany what's about the public coverage reports that @JaimeMartin mentioned? Can you provide us a link or suggest the changes?

@MiguelCompany
Copy link
Collaborator

I accidentally published my comment unfinished. It's been edited.

@ahcorde
Copy link
Contributor

ahcorde commented May 8, 2020

do you have these lines explicity written somewhere in your documentation?

API breaks should only be allowed between major version changes, if an ABI break is required, it should be done between minor versions and be clearly stated on the release notes.

@MiguelCompany
Copy link
Collaborator

do you have these lines explicity written somewhere in your documentation?

It is here

@EduPonz
Copy link

EduPonz commented Sep 23, 2020

Hi @Blast545 @ahcorde @brawner ,

At eProsima, we have been working on Fast DDS Quality level and we'd really appreciate your input in a collection of pull requests that in our opinion qualify for a Quality Level 2 once merged. Those are:

Please mind that this is a work in review, some links in the documents do not work yet. For those cases, the PR descriptions link to the relevant documents. Feel free to comment and make suggestions in any of the related PRs. Once we have those sorted out, we'll be ready to update this PR to a Q2 for Fast DDS.

Thanks!

@EduPonz
Copy link

EduPonz commented Sep 28, 2020

Hi @Blast545 @ahcorde @brawner ,

All our quality declarations have been merged now, with the exception of Fast DDS' (eProsima/Fast-DDS#1400), which @brawner already approved. If it's OK with you guys we'll merge that too, and then this PR should be updated to point to the QD of its dependencies directly (instead of stating them here). All in all, Fast DDS qualifies now for a Quality Level 2, and so this this package.

@Blast545 Could you update the PR to reflect those changes? IMHO it's the only thing left to merge the PR.

Thanks!

@chapulina
Copy link

Fast DDS qualifies now for a Quality Level 2

That's great news, congrats!

point to the QD of its dependencies directly

Agreed, we don't need a QD for FastDDS in this repo, we should link to the one upstream.

@EduPonz
Copy link

EduPonz commented Jan 19, 2021

Due to Fast DDS reaching QL1 (eProsima/Fast-DDS#1610), I think this PR can be closed

@Blast545 Blast545 closed this Jan 21, 2021
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.

7 participants