-
Notifications
You must be signed in to change notification settings - Fork 445
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
extensions: support for content sharing for ROS #4221
Conversation
Not sure why |
Tracking with CRAFT-1848 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I have no concerns with the design or implementation. My feedback is minor and related to linters and best practices.
I see two blockers:
- You will need to resolve the conflicts with the
main
branch - You will need to rebase onto Sergio's upcoming feature to make extensions part plugin aware (scheduled for this pulse)
Would you prefer that I merge or rebase? |
No preference. It will get squash-merged in the end, so whichever path is easier for you is fine. |
#4271 contains the functionality for extensions to be part plugin aware |
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
ad72ee5
to
2fd47e4
Compare
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
2fd47e4
to
2d5b98a
Compare
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## main #4221 +/- ##
==========================================
- Coverage 89.20% 89.13% -0.07%
==========================================
Files 300 315 +15
Lines 20639 21037 +398
==========================================
+ Hits 18410 18752 +342
- Misses 2229 2285 +56
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
The PR was rebased on top of Note that I had to disable the |
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
Signed-off-by: artivis <jeremie.deray@canonical.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, I have no further concerns.
@sergiusens @mr-cal what's the ETA on merging this? can we help somehow? |
@sergiusens any update here? |
Hey @MirkoFerrati, sorry I missed your ping. Since Sergio and I both approved this, I'll get it merged today. |
@MirkoFerrati sorry about that, this should have been merged when I merged back main back then |
@sergiusens no worries, I didn't want to spam with the pings as I didn't know the status. Next time I see "main" merged in the PR branch I will know how it works and ping you more often! Thanks for the fast response today! |
@MirkoFerrati no sweat, re-requesting review also works as it shows up in our review queue more prominently |
This PR introduces the support of content sharing for ROS together with a few extensions that make building a ROS content sharing snap seamless.
To that effect several snaps meant to distribute ROS through content-sharing were created and are available on the store (hidden, e.g.
ros-foxy-ros-base
&ros-foxy-ros-base-dev
)This PR is large as it concerns,
To note:
requires the 'dev' snap providing the build-time material to execute a script listing all ROS packages it contains when it's built
Have you followed the guidelines for contributing?
Have you signed the CLA?
Have you successfully run
make lint
?Have you successfully run
pytest tests/unit
?