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

Exit with error on malformed compose version #6480

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@collin5
Copy link
Contributor

commented Jan 23, 2019

Resolves #6036

@collin5 collin5 force-pushed the collin5:b6036 branch 2 times, most recently from 446eda4 to f845715 Jan 23, 2019

@ulyssessouza

This comment has been minimized.

Copy link
Collaborator

commented Jan 25, 2019

Thank you @collin5 !

nit: Could you please squash the 2 commits?

@collin5 collin5 force-pushed the collin5:b6036 branch from f845715 to 9821421 Jan 25, 2019

@collin5

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

Done! Thank you.

@@ -208,7 +208,7 @@ def version(self):
'Version in "{}" is invalid - it should be a string.'
.format(self.filename))

if version == '1':
if version == '1' or not all((n.isdigit() for n in version.split('.'))):

This comment has been minimized.

Copy link
@ulyssessouza

ulyssessouza Jan 25, 2019

Collaborator

nit: Any special reason for the extra parentheses inside all() ?

This comment has been minimized.

Copy link
@collin5

collin5 Jan 25, 2019

Author Contributor

Was passing the collection to all() as a generator. Uh! 🤦‍♂️ , Just realized it's actually not required. Let me update this ASAP. Thanks for catching this.

exit with error on malformed compose version
Signed-off-by: Collins Abitekaniza <abtcolns@gmail.com>

unit test malformed compose version

Signed-off-by: Collins Abitekaniza <abtcolns@gmail.com>

@collin5 collin5 force-pushed the collin5:b6036 branch from 9821421 to 4649d00 Jan 25, 2019

@collin5

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

Fixed. Thanks!

@collin5

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

@ulyssessouza 🙋‍♂️

@ulyssessouza ulyssessouza self-requested a review Apr 9, 2019

@ulyssessouza
Copy link
Collaborator

left a comment

What about a function checkVersion to check cases like version: '0'? And also, I think that instead of rejecting the case version: '3 ' it would be better just to trim the spaces and accept as version 3.
This would be a more user-friendly approach.

@ulyssessouza ulyssessouza self-requested a review Apr 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.