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.

Sign up for GitHub

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

Jump to bottom

Add list-unit-files command to bluechictl #915

Merged
merged 5 commits into from
Aug 7, 2024
Merged

Add list-unit-files command to bluechictl #915

merged 5 commits into from
Aug 7, 2024

Conversation

trev-allison03
Copy link
Contributor

@trev-allison03 trev-allison03 commented Jul 9, 2024 •
edited
Loading

Add list unit files methods to agent/agent.c, controller/node.c, controller/controller.c
Add list-unit-files command to bluechitctl and man page

Fixes: #889

@coveralls
Copy link

coveralls commented Jul 10, 2024 •
edited
Loading

Coverage Status

coverage: 84.765% (-0.4%) from 85.154%
when pulling b06e4f1 on trev-allison03:main
into 5fc35bf on eclipse-bluechi:main.

@trev-allison03 trev-allison03 force-pushed the main branch 2 times, most recently from 31dc4a1 to febfef0 Compare July 11, 2024 13:20
@engelmi
Copy link
Member

engelmi commented Jul 11, 2024

Hi @trev-allison03 ,
Thanks a lot for your contribution!
Bluechi is part of Eclipse Foundation, so we need to follow the contribution guidelines and as such we can only accept contributions from persons who signed the Eclipse Contribution Agreement: https://www.eclipse.org/legal/ECA.php
So could you please create an Eclipse account and sign ECA there?

@trev-allison03 trev-allison03 force-pushed the main branch 3 times, most recently from c17a081 to 3cf15bb Compare July 17, 2024 15:54
@trev-allison03 trev-allison03 changed the title Add list unit files methods to agent and node Add list unit files methods to controller, node, and agent Jul 18, 2024
@trev-allison03 trev-allison03 force-pushed the main branch 3 times, most recently from 3e3433e to 959bd81 Compare July 23, 2024 19:31
mwperina
mwperina requested changes Jul 24, 2024
View reviewed changes
Copy link
Member

@mwperina mwperina left a comment

Choose a reason for hiding this comment

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

Please also recheck formatting, linter complains about following files:

File reformatted:  /__w/bluechi/bluechi/src/libbluechi/bus/utils.h
File reformatted:  /__w/bluechi/bluechi/src/libbluechi/bus/utils.c
File reformatted:  /__w/bluechi/bluechi/src/client/method-list-unit-files.c
File reformatted:  /__w/bluechi/bluechi/src/client/main.c

@trev-allison03 trev-allison03 force-pushed the main branch 4 times, most recently from 134c959 to 199d1a3 Compare July 25, 2024 15:09
@trev-allison03 trev-allison03 changed the title Add list unit files methods to controller, node, and agent Add list-unit-files command to bluechictl Jul 25, 2024
@mwperina mwperina marked this pull request as ready for review July 25, 2024 18:54
mwperina
mwperina approved these changes Jul 26, 2024
View reviewed changes
Copy link
Member

@mwperina mwperina left a comment

Choose a reason for hiding this comment

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

LGTM

engelmi
engelmi requested changes Jul 26, 2024
View reviewed changes
Copy link
Member

@engelmi engelmi left a comment

Choose a reason for hiding this comment

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

Overall looks good.
Great work!

mkemel
mkemel reviewed Jul 28, 2024
View reviewed changes
Copy link
Member

@mkemel mkemel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great work Trevor!

This is quite a bunch of new code that is not covered by integration tests (see coveralls' comment above). These test the functionality, and also run with valgrind to check for memory leaks. Usually it is best to add an integration test together with the new code, but it's also okay to merge this and then add the test. If you choose to do so, please at least test it locally with valgrind before we merge.

@trev-allison03 trev-allison03 force-pushed the main branch 6 times, most recently from dd03d03 to 978cf58 Compare July 31, 2024 17:49
@engelmi engelmi mentioned this pull request Jul 31, 2024
Closed
6 tasks
mwperina
mwperina reviewed Aug 1, 2024
View reviewed changes
Copy link
Member

@mwperina mwperina left a comment

Choose a reason for hiding this comment

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

Except small nitpick the PR looks good, great work!

@trev-allison03 trev-allison03 force-pushed the main branch 3 times, most recently from 9e23472 to 8be6c8a Compare August 6, 2024 15:32
mwperina
mwperina approved these changes Aug 7, 2024
View reviewed changes
Copy link
Member

@mwperina mwperina left a comment

Choose a reason for hiding this comment

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

LGTM

mkemel
mkemel approved these changes Aug 7, 2024
View reviewed changes
Copy link
Member

@mkemel mkemel left a comment

Choose a reason for hiding this comment

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

LGTM.
Really great work!

engelmi
engelmi approved these changes Aug 7, 2024
View reviewed changes
Copy link
Member

@engelmi engelmi left a comment

Choose a reason for hiding this comment

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

LGTM, nice work!

CI step for the API check is failing due to version mismatch. Lets fix that in a new PR.

@engelmi engelmi merged commit 5560c36 into eclipse-bluechi:main Aug 7, 2024
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mwperina mwperina mwperina approved these changes

@engelmi engelmi engelmi approved these changes

@mkemel mkemel mkemel approved these changes

@alexlarsson alexlarsson Awaiting requested review from alexlarsson alexlarsson is a code owner

@rhatdan rhatdan Awaiting requested review from rhatdan rhatdan is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RFE: Add ListUnitFiles API
5 participants
@trev-allison03 @coveralls @engelmi @mwperina @mkemel