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

Review CI by correcting the configurations #71

Merged
merged 58 commits into from
Apr 13, 2021
Merged

Conversation

destogl
Copy link
Contributor

@destogl destogl commented Apr 10, 2021

PR against destogl/edit-non-functional-files (#70) for better changes overview.

It should be rebased after (#70) is merged. But it can be already reviewed.

@destogl destogl changed the base branch from main to develop April 10, 2021 08:33
@destogl destogl marked this pull request as draft April 10, 2021 08:33
@destogl destogl marked this pull request as ready for review April 13, 2021 14:25
@destogl
Copy link
Contributor Author

destogl commented Apr 13, 2021

@AndyZe this can be reviewed after #66 is merged. It is expected that "Format" fails. The correction will be done in the follow-up PR.

@AndyZe
Copy link
Contributor

AndyZe commented Apr 13, 2021

@denis I tried to rebase this but it has almost diverged too far to do manually. Can you handle that?

Or, you might want to create a new branch (starting from develop) and cherry-pick the new commits you want to keep. That would probably be easier. Then make a new PR

@destogl
Copy link
Contributor Author

destogl commented Apr 13, 2021

I commented out the dependency on ur_client_library because it can not be found by rosdep. We are not exporting it properly. Opening follow-up issue for this.

@destogl
Copy link
Contributor Author

destogl commented Apr 13, 2021

The coverage build has issues with the ur_client_library, but industrial_ci is working. We should go for merge...

Copy link
Contributor

@AndyZe AndyZe left a comment

Choose a reason for hiding this comment

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

I'll push a commit to clean up my minor nitpicks

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
LICENSE Outdated
Comment on lines 178 to 202

APPENDIX: How to apply the Apache License to your work.

To apply the Apache License to your work, attach the following
boilerplate notice, with the fields enclosed by brackets "[]"
replaced with your own identifying information. (Don't include
the brackets!) The text should be enclosed in the appropriate
comment syntax for the file format. We also recommend that a
file or class name and description of purpose be included on the
same "printed page" as the copyright notice for easier
identification within third-party archives.

Copyright [yyyy] [name of copyright owner]

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something people normally do with the Apache license? I'm not familiar with it but it seems unusual to modify the license file. Think I will ask @tylerjw to review this PR

Copy link

Choose a reason for hiding this comment

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

Please generate the license with ament_copyright adding the copyright holder and year correctly.

ur_controllers/package.xml Show resolved Hide resolved
ur_robot_driver/package.xml Show resolved Hide resolved
@AndyZe AndyZe requested a review from tylerjw April 13, 2021 20:45
@AndyZe
Copy link
Contributor

AndyZe commented Apr 13, 2021

@tylerjw can you please review the license and CI stuff?

@tylerjw
Copy link

tylerjw commented Apr 13, 2021

Please fix the Format errors. Here is a guide for using pre-commit to fix those issues: https://moveit.ros.org//documentation/contributing/code/

@destogl
Copy link
Contributor Author

destogl commented Apr 13, 2021

Please fix the Format errors. Here is a guide for using pre-commit to fix those issues: https://moveit.ros.org//documentation/contributing/code/

I want to fix them in the separate PR to keep them smaller.

@destogl destogl merged commit 3790877 into develop Apr 13, 2021
@delete-merged-branch delete-merged-branch bot deleted the destogl/revive-ci branch April 13, 2021 21:23
Copy link

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

I don't understand not fixing the pre-commit errors (outside of the catkin_lint errors) or not squashing this before merging. The 58 commits are completely unnecessary (this is a ~300 line pr) and most of the pre-commit errors are fixed automatically by just running pre-commit. The catkin_lint ones would be more work but I still would have wanted to see them fixed.

😕

@@ -187,7 +187,7 @@
same "printed page" as the copyright notice for easier
identification within third-party archives.

Copyright [yyyy] [name of copyright owner]
Copyright 2021 PickNik, Inc.
Copy link

Choose a reason for hiding this comment

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

This should be PickNik Inc. (no comma) as @nbbrooks had me do for moveit2 as that is the legal name we should use.

AndyZe pushed a commit that referenced this pull request Apr 14, 2021
* Update license.

* Update CI configs.

Co-authored-by: Lovro <lovro.ivanov@gmail.com>
Co-authored-by: AndyZe <zelenak@picknik.ai>
AndyZe pushed a commit that referenced this pull request Apr 14, 2021
* Update license.

* Update CI configs.

Co-authored-by: Lovro <lovro.ivanov@gmail.com>
Co-authored-by: AndyZe <zelenak@picknik.ai>
AndyZe pushed a commit that referenced this pull request Apr 14, 2021
* Update license.

* Update CI configs.

Co-authored-by: Lovro <lovro.ivanov@gmail.com>
Co-authored-by: AndyZe <zelenak@picknik.ai>
andy-Chien pushed a commit to NanyangBot/Universal_Robots_ROS2_Driver that referenced this pull request Oct 5, 2022
* Update license.

* Update CI configs.

Co-authored-by: Lovro <lovro.ivanov@gmail.com>
Co-authored-by: AndyZe <zelenak@picknik.ai>
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.

None yet

4 participants