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

Forbid empty string values for container id in commands #305

Merged
merged 1 commit into from
Sep 18, 2021

Conversation

YJDoc2
Copy link
Collaborator

@YJDoc2 YJDoc2 commented Sep 18, 2021

Currently container_id is a required argument for all commands that require container_id, but there is no restriction that empty string value cannot be provided as container_id. For example :

youki --root (root_path) create --bundle (bundle path)

gives error, but

youki --root (root_path) create --bundle (bundle path) ""

does not, as (at least bash) passes "" as an empty string argument, and clap allows it, as it is a valid string value.
This PR adds forbid_empty_value to all container_id fields, which forces it to give an error message whenever empty string is provided.

@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Sep 18, 2021

Please take a look @utam0k @Furisto

@codecov-commenter
Copy link

Codecov Report

Merging #305 (54811bb) into main (71ddb34) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #305      +/-   ##
==========================================
+ Coverage   77.17%   77.19%   +0.01%     
==========================================
  Files          43       43              
  Lines        6139     6139              
==========================================
+ Hits         4738     4739       +1     
+ Misses       1401     1400       -1     

@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Sep 18, 2021

Also, here changes job is not run even though there are changes in src/* file. this is probably due to
https://github.com/containers/youki/blob/main/.github/workflows/main.yml#L20
where the src/* is in quotes ('') due to which it does not match any path.
@utam0k @Furisto Should I try updating the workflow file in this PR only, or should I open a new PR for that?

@utam0k
Copy link
Member

utam0k commented Sep 18, 2021

Also, here changes job is not run even though there are changes in src/* file. this is probably due to
https://github.com/containers/youki/blob/main/.github/workflows/main.yml#L20
where the src/* is in quotes ('') due to which it does not match any path.
@utam0k @Furisto Should I try updating the workflow file in this PR only, or should I open a new PR for that?

Can I ask you for another PR?

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.

a nice improvement!

@utam0k utam0k merged commit 74ed630 into containers:main Sep 18, 2021
@YJDoc2 YJDoc2 deleted the make_id_nonempty branch September 25, 2021 04:32
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

3 participants