add colcon-mixin-name-build and colcon-mixin-name-test overrides #526

ddengster · 2021-02-09T03:37:30Z

Fixes #525

codecov · 2021-02-09T03:38:20Z

Codecov Report

Merging #526 (3cc17df) into master (cf1700f) will decrease coverage by 2.23%.
The diff coverage is 13.33%.

@@            Coverage Diff             @@
##           master     #526      +/-   ##
==========================================
- Coverage   42.42%   40.19%   -2.24%     
==========================================
  Files           2        2              
  Lines         198      209      +11     
  Branches       36       40       +4     
==========================================
  Hits           84       84              
- Misses        114      125      +11

Impacted Files	Coverage Δ
src/action-ros-ci.ts	`30.55% <13.33%> (-1.99%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf1700f...3cc17df. Read the comment docs.

Signed-off-by: ddengster <ed.fan@osrfoundation.org>

.github/workflows/test.yml

src/action-ros-ci.ts

christophebedard · 2021-02-09T14:58:03Z

action.yml

+  colcon-mixin-name-test:
+    default: ""
+    description: "Colcon mixin to be used to as input to colcon test(empty means no mixin). Overrides colcon-mixin-name"
+    required: false


It wouldn't be backwards compatible, but maybe colcon-mixin-name should just be removed in this case.

yea it'd be a breaking change; i'm content with leaving it be for now. let me know if you'd like this change in the PR

I think either way should be fine. We can always remove colcon-mixin-name later and avoid breaking changes for now.

I'll let the people who actually have approval rights/write access take it from here and decide.

christophebedard · 2021-02-10T15:19:03Z

PS. how do get CI to run the updated tests? It doesnt seem to be running those

it does run them: https://github.com/ros-tooling/action-ros-ci/pull/526/checks?check_run_id=1868379801#step:14:3

btw you'll have to either sign-off the two commits resulting from my code suggestions, or just squash everything and sign-off

Signed-off-by: ddengster <ed.fan@osrfoundation.org>

Co-authored-by: Christophe Bedard <bedard.christophe@gmail.com> Signed-off-by: ddengster <ed.fan@osrfoundation.org>

emersonknapp · 2021-02-12T05:39:11Z

Addresses issue #525

Btw if you change this to "Fixes #x" or "Closes #x" then github can auto-close for you - it only accepts a few specific verbs.

Sorry for being a little slow to get to this, but per my comment #525 (comment) - I think this API could be more generic (and make the API actually be smaller overall). What do you think about this suggestion? I personally like it better than adding more specific arguments, as that can get out of control as we add more access points. It's the approach used in https://github.com/ros-tooling/cross_compile#package-selection-and-build-customization

ddengster · 2021-02-26T03:47:44Z

Sorry for being a little slow to get to this, but per my comment #525 (comment) - I think this API could be more generic (and make the API actually be smaller overall). What do you think about this suggestion? I personally like it better than adding more specific arguments, as that can get out of control as we add more access points. It's the approach used in https://github.com/ros-tooling/cross_compile#package-selection-and-build-customization

See reply #525 (comment). Should we merge this PR if there's no solution there?

emersonknapp · 2021-03-02T03:18:47Z

Thanks for the patience - I was able to get a proof of concept solution working that I think is acceptable.

PR here #555 - see the PR description for what's going on. Specifically check out the run output here https://github.com/ros-tooling/action-ros-ci/pull/555/checks?check_run_id=2009781903#step:6:28

As long as we document the usage - "Use a leading | for multiline, must be valid JSON", I think this solution could work really well. What do you think?

christophebedard · 2021-03-02T16:26:56Z

That looks nice. Since there's overlap between the settings/values in a colcon defaults file and the action's inputs, do we offer both options and have one override the other, e.g. colcon-defaults overrides the related action inputs?

I'm asking this because having multiple ways of doing something is generally not a good thing, UX-wise. Perhaps we should open a separate issue to talk about this.

emersonknapp · 2021-03-02T17:18:39Z

That looks nice. Since there's overlap between the settings/values in a colcon defaults file and the action's inputs, do we offer both options and have one override the other, e.g. colcon-defaults overrides the related action inputs?

I'm asking this because having multiple ways of doing something is generally not a good thing, UX-wise. Perhaps we should open a separate issue to talk about this.

Yes, I'm thinking it would be best to phase out the duplicated options. For now, defaults will always be overridden by an explicit command line option, so there is a clear behavior

Relates to #525, #526, #555 This adds a `colcon-defaults` input. The JSON content is validated and then written to `COLCON_DEFAULTS_FILE` Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>

christophebedard · 2021-03-11T21:07:14Z

@emersonknapp what should we do with this PR and #525? It's now possible to do what this PR aims to enable (use coverage-gcc without having a colcon error on newer versions), but if users use the colcon-mixin-name option it can lead to an error, which isn't good.

Personally, maybe overriding what I said above, I'd be fine with merging this PR, since using mixins is a common enough use-case. We could draw the line here (wrt duplicate options/flags in inputs and colcon-defaults), at least for now.

emersonknapp · 2021-03-11T21:24:24Z

I would prefer to have users use the colcon defaults option for all mixins now - I'd rather remove colcon-mixin-name than add a new option here

emersonknapp · 2021-03-23T19:26:31Z

Use case implemented by #591

ddengster requested a review from a team as a code owner February 9, 2021 03:37

ddengster requested review from Karsten1987 and jaisontj and removed request for a team February 9, 2021 03:37

ddengster added 3 commits February 9, 2021 11:41

add colcon-mixin-name-build and colcon-mixin-name-test overrides

eb3c7cc

Signed-off-by: ddengster <ed.fan@osrfoundation.org>

add spaces

540b65e

Signed-off-by: ddengster <ed.fan@osrfoundation.org>

add boolean, clean string concats

d7f104c

Signed-off-by: ddengster <ed.fan@osrfoundation.org>

ddengster mentioned this pull request Feb 9, 2021

[wip] use action-ros-ci, add codecoverage osrf/rmf_core#285

Draft

christophebedard reviewed Feb 9, 2021

View reviewed changes

ddengster and others added 4 commits February 11, 2021 15:17

add mixin override test

64c61e3

Signed-off-by: ddengster <ed.fan@osrfoundation.org>

clarify test_mixin_test_overrides2 name

49090ea

Signed-off-by: ddengster <ed.fan@osrfoundation.org>

fix indent

f9182de

Signed-off-by: ddengster <ed.fan@osrfoundation.org>

fix step test_mixin name #2

1b6d449

Co-authored-by: Christophe Bedard <bedard.christophe@gmail.com> Signed-off-by: ddengster <ed.fan@osrfoundation.org>

Merge branch 'master' into master

3cc17df

emersonknapp mentioned this pull request Mar 2, 2021

[test] Get multiline action input - parse as JSON #555

Closed

christophebedard mentioned this pull request Mar 7, 2021

Add colcon-defaults input #570

Merged

christophebedard mentioned this pull request Mar 11, 2021

Tag a new release: 0.2.0 #576

Closed

christophebedard mentioned this pull request Mar 21, 2021

Remove colcon-mixin-name #591

Merged

emersonknapp closed this Mar 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add colcon-mixin-name-build and colcon-mixin-name-test overrides #526

add colcon-mixin-name-build and colcon-mixin-name-test overrides #526

ddengster commented Feb 9, 2021 •

edited

Loading

codecov bot commented Feb 9, 2021 •

edited

Loading

christophebedard Feb 9, 2021

ddengster Feb 10, 2021 •

edited

Loading

christophebedard Feb 10, 2021 •

edited

Loading

christophebedard commented Feb 10, 2021

emersonknapp commented Feb 12, 2021

ddengster commented Feb 26, 2021 •

edited

Loading

emersonknapp commented Mar 2, 2021

christophebedard commented Mar 2, 2021

emersonknapp commented Mar 2, 2021

christophebedard commented Mar 11, 2021

emersonknapp commented Mar 11, 2021

emersonknapp commented Mar 23, 2021

add colcon-mixin-name-build and colcon-mixin-name-test overrides #526

add colcon-mixin-name-build and colcon-mixin-name-test overrides #526

Conversation

ddengster commented Feb 9, 2021 • edited Loading

codecov bot commented Feb 9, 2021 • edited Loading

Codecov Report

christophebedard Feb 9, 2021

Choose a reason for hiding this comment

ddengster Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

christophebedard Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

christophebedard commented Feb 10, 2021

emersonknapp commented Feb 12, 2021

ddengster commented Feb 26, 2021 • edited Loading

emersonknapp commented Mar 2, 2021

christophebedard commented Mar 2, 2021

emersonknapp commented Mar 2, 2021

christophebedard commented Mar 11, 2021

emersonknapp commented Mar 11, 2021

emersonknapp commented Mar 23, 2021

ddengster commented Feb 9, 2021 •

edited

Loading

codecov bot commented Feb 9, 2021 •

edited

Loading

ddengster Feb 10, 2021 •

edited

Loading

christophebedard Feb 10, 2021 •

edited

Loading

ddengster commented Feb 26, 2021 •

edited

Loading