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

Add types to Action with rhel roscli fix #1361

Open
wants to merge 20 commits into
base: rolling
Choose a base branch
from

Conversation

InvincibleRMC
Copy link
Contributor

Adds type to actions. A do over of #1349. I believe I have fixed the bug causing rhel roscli to fail.

@InvincibleRMC
Copy link
Contributor Author

@fujitatomoya When you get a chance can you run rhel CI on this to see if the fix works?

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

@InvincibleRMC thank you very much for being patient and keep making contribution for this type checks. we really appreciate that!

I will start the CI now!

just out of curiosity, i would like to learn which part was failing the RHEL with previous PR. could you point out the source code and share your thoughts?

@fujitatomoya
Copy link
Collaborator

Pulls: #1361
Gist: https://gist.githubusercontent.com/fujitatomoya/f6e3fb454459539dd4247706ad5abd87/raw/8844303445a5a3b903daea42dbae4862b475ffc4/ros2.repos
BUILD args: --packages-above-and-dependencies rclpy
TEST args: --packages-above rclpy
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14592

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@InvincibleRMC
Copy link
Contributor Author

I don't imagine the changes I made would cause the security failings in linux but, probably wise to run CI again to be sure?

@fujitatomoya
Copy link
Collaborator

  • Linux Build Status

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

@InvincibleRMC thanks for sharing, this looks good to me. and CI is green. but in this time, i would like to have 2nd review from other PMC members. please wait for a while.

rclpy/rclpy/action/client.py Show resolved Hide resolved
@sloretz
Copy link
Contributor

sloretz commented Nov 1, 2024

@Mergifyio update

Copy link
Contributor

mergify bot commented Nov 1, 2024

update

✅ Branch has been successfully updated

@sloretz
Copy link
Contributor

sloretz commented Nov 1, 2024

Pulls: #1361
Gist: https://gist.githubusercontent.com/sloretz/c82fca564421fe672a5953d2363646b8/raw/e222c0a533dfb4cd9bde4ae4fe03dad53bcffb0b/ros2.repos
BUILD args: --packages-above-and-dependencies rclpy
TEST args: --packages-above rclpy
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14772

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@InvincibleRMC
Copy link
Contributor Author

@sloretz Do you need anything else from me?

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.

3 participants