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

Upgrade clap to v4 #1443

Merged
merged 2 commits into from
Dec 30, 2022
Merged

Upgrade clap to v4 #1443

merged 2 commits into from
Dec 30, 2022

Conversation

Overflow0xFFFF
Copy link
Contributor

Description

The major update of clap from v3 to v4 comes with code-breaking changes.

See #1236.

Proposed changes

Refactor youki's CLI just enough to bring in clap v4 and allow dependabot to manage the dependency once more.
In order to upgrade clap, I had followed the migration guide:

https://github.com/clap-rs/clap/blob/master/CHANGELOG.md#400---2022-09-28

This upgrade does change the --help output a little bit. I assume this is the full list of changes, but I could be mistaken:

clap-rs/clap#4132

As part of my testing, I ran make test-all and followed the user guide to build and run a container. Some tests are failing locally for me, but these same tests are failing for me on the main branch. I haven't yet determined if that is due to my local development environment (Fedora 36) or not.

@codecov-commenter
Copy link

codecov-commenter commented Dec 24, 2022

Codecov Report

Merging #1443 (a3bfc10) into main (546efca) will decrease coverage by 0.13%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1443      +/-   ##
==========================================
- Coverage   68.93%   68.79%   -0.14%     
==========================================
  Files         120      120              
  Lines       13055    13082      +27     
==========================================
+ Hits         8999     9000       +1     
- Misses       4056     4082      +26     

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Dec 28, 2022

Hey @Overflow0xFFFF Thanks a lot for PR. Sorry for delay in checking.
Can you let know which tests are failing for you, so we can see what is the issue (because CI is passing all tests)
Also, when you say that you refactored the CLI, can you tell what changes (apart from the ones in clap builder args) have you made?

Also please sign your PR, so DCO check can pass.
Thanks!

Signed-off-by: Joshua Ford <joshua.ford@protonmail.com>
Signed-off-by: Joshua Ford <joshua.ford@protonmail.com>
@Overflow0xFFFF
Copy link
Contributor Author

Thank you for the feedback, @YJDoc2!

I'm seeing odd test failures. The one that appears most often is test utils::tests::test_is_executable. This one seems to fail every time when running make test-all. I see it whenever I run make unittest and make featuretest directly. When running make oci-tests, I see this error message:

Running delete_only_create_resources/delete_only_create_resources.t
TAP version 13
open /sys/fs/cgroup/pids/cgrouptest/tasks: no such file or directory

Since these same tests seem to run fine in the Ubuntu-based GitHub Actions workflow, I think I have to conclude that it might be my development environment running on Fedora 36. I noticed that the Vagrantfile currently uses a Fedora 33 image, which is no longer available. Might be a good candidate for another contribution?

As for refactoring the CLI, I was very careful not to make any unnecessary changes. The only refactoring I needed to do was to change out some deprecated macros for their replacements. The only place I'm uncertain about is in tests/rust-integration-tests/integration_test/src/main.rs because the first CI run showed a failure in the integration tests.

@utam0k
Copy link
Member

utam0k commented Dec 29, 2022

Thank you for the feedback, @YJDoc2!

I'm seeing odd test failures. The one that appears most often is test utils::tests::test_is_executable. This one seems to fail every time when running make test-all. I see it whenever I run make unittest and make featuretest directly. When running make oci-tests, I see this error message:

Running delete_only_create_resources/delete_only_create_resources.t
TAP version 13
open /sys/fs/cgroup/pids/cgrouptest/tasks: no such file or directory

Since these same tests seem to run fine in the Ubuntu-based GitHub Actions workflow, I think I have to conclude that it might be my development environment running on Fedora 36. I noticed that the Vagrantfile currently uses a Fedora 33 image, which is no longer available. Might be a good candidate for another contribution?

As for refactoring the CLI, I was very careful not to make any unnecessary changes. The only refactoring I needed to do was to change out some deprecated macros for their replacements. The only place I'm uncertain about is in tests/rust-integration-tests/integration_test/src/main.rs because the first CI run showed a failure in the integration tests.

Thanks for your PR. I think this PR is going to be fixed by #1406

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

Thanks a lot 🙏

@utam0k utam0k merged commit bcd90bb into youki-dev:main Dec 30, 2022
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.

4 participants